-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix all invalid escape sequences #49004
fix all invalid escape sequences #49004
Conversation
Signed-off-by: Fabian Keller <[email protected]>
Signed-off-by: Fabian Keller <[email protected]>
4e7e167
to
ee9f0c7
Compare
Signed-off-by: Fabian Keller <[email protected]>
@bluenote10 thank you very much for doing the clean up! Can you please take a look at the CI failures? |
Signed-off-by: Fabian Keller <[email protected]>
Signed-off-by: Fabian Keller <[email protected]>
With pleasure 😉
Good catch: There were actually a few regexes where the |
@@ -26,7 +26,7 @@ | |||
|
|||
@DeveloperAPI | |||
class DoublyRobust(OffPolicyEstimator): | |||
"""The Doubly Robust estimator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, these aren't regexes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but docstrings can also contain invalid escape sequences. See for instance the noise coming out of mypy in #48921, which points to malformed docstrings.
I would assume that most likely these docstrings were written thinking that a \
is just a plain \
, which raw string literals achieve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but the solution isn't to annotate it as regex though, right?
Solution should be to escape the slash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "annotate it as regex"?
Using raw string literals like r"""...""" has nothing to do with regexes. They are just string literals where the \
is not used as an escape code for introducing special chars like \n
or \r
.
It just happens to be common to use raw string literals when writing regexes, because it often involves \
and the raw string literal avoid having to escape them all.
@alexeykudinkin could you take another pass at this? |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.
|
Thanks a lot for working on this @bluenote10, let's get this merged if the tests pass :) |
@bluenote10 Thanks a lot for your contribution, and sorry for the delay in getting this merged 🎉 |
Why are these changes needed?
This PR fixes all invalid escape sequences, and enables the corresponding flake8 rule
W605
.This came up as part of #48921, leading to the wrong conclusion that the mypy hiccup is related to invalid escape sequences. Most likely it isn't (python/mypy#18215 (comment)), but it may still be a good idea to get rid of these to avoid unnecessary warnings and confusion.
I couldn't really figure out how flake8 is executed by the CI, because just running
flake8
on themaster
branch actually fails, and runningscripts/format.sh
doesn't seem to do anything (most likely because it only checks the diff, and isn't a full check). So I'm wondering if enabling theW605
check currently actually has much of an effect.Related issue number
Related to #48921
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.