Skip to content
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 escape sequences #591

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix escape sequences #591

wants to merge 1 commit into from

Conversation

NeilGirdhar
Copy link
Contributor

@NeilGirdhar NeilGirdhar commented Apr 25, 2024

Fixes #598

@NeilGirdhar
Copy link
Contributor Author

@mblondel Any chance we can get this in soon?

@NeilGirdhar
Copy link
Contributor Author

On Python 3.12, this gives:

.../.venv/lib/python3.12/site-packages/jaxopt/_src/osqp.py:299: SyntaxWarning: invalid escape sequence '\m'

@stephen-huan
Copy link

@NeilGirdhar it should also be fixed here.

'ignore_pattern': '_test\.py', # no gallery for test of examples

+1 for merging, this breaks GPJax's tests for me.

@NeilGirdhar
Copy link
Contributor Author

@NeilGirdhar it should also be fixed here.

'ignore_pattern': '_test\.py', # no gallery for test of examples

+1 for merging, this breaks GPJax's tests for me.

Do you mind doing that in separate PR? I don't use jaxopt anymore and I don't want to deal with the added back-and-forth :/ I think that one should a raw string if it's being passed to re, but I don't want to take the time to figure it out :)

@stephen-huan
Copy link

A raw string is a Python syntactic construct to create normal strings, but the result is just a normal string once it's passed to a regex function (that is, there's no such thing as a "raw string" type as far as I'm aware). What's shown above is just a normal string, and the same warning applies. (I found this error automatically with pyright). Compare

>>> r"_test\.py"
'_test\\.py'
>>> type(r"_test\.py")
<class 'str'>
>>> "_test\.py"
<stdin>:1: SyntaxWarning: invalid escape sequence '\.'
'_test\\.py'
>>> "_test\.py" == "_test\\.py" == r"_test\.py"
<stdin>:1: SyntaxWarning: invalid escape sequence '\.'
True

This is minor though, since conf.py is some sort of internal documentation generator anyways.

@NeilGirdhar
Copy link
Contributor Author

Yes, I know. The reason it should be an r-string is because conventionally (according to linters like Ruff), r-strings should be passed as the match pattern of an re-match regardless of whether or not they take advantage of any of differences. So, your first way of writing it is the conventional way that it should be written rather than the equivalent non-r-string.

@stephen-huan
Copy link

stephen-huan commented Dec 3, 2024

Which ruff rule are you referring to?

import re

re.match("_test\\.py", "_test\\.py")
ruff check test.py
All checks passed!

Whether it's r"_test\.py" or "_test\\.py" is immaterial to me since they're the same. It just can't be "_test\.py".

@NeilGirdhar
Copy link
Contributor Author

NeilGirdhar commented Dec 3, 2024

RUF039

Whether it's r"_test.py" or "_test\.py" is immaterial to me since they're the same.

I think the logic behind the rule is that it's a bug magnet if the pattern changes, and in this case, it's easier to read without the double-escaping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Annoying warning in 3.12
2 participants