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

MNT: Remove Python 2 leftovers #1116

Merged
merged 2 commits into from Mar 30, 2023
Merged

Conversation

StefRe
Copy link
Contributor

@StefRe StefRe commented Mar 28, 2023

to improve readability and unify coding style (older code used %, newer contributions use format etc). Modifications were done using pyupgrade.

Files in tinybuild\examples\future were deliberately left unchanged.

I know it's a lot of changed files but I think it greatly improves readability and the Python docs also recommend the use of format over %.

@larsoner
Copy link
Contributor

and the Python docs also recommend the use of format over %.

I actually prefer f-strings nowadays. Is it simple to change to that?

@StefRe
Copy link
Contributor Author

StefRe commented Mar 28, 2023

Is it simple to change to that?

I think so, using the --py36-plus option (which will also change some other things but these changes can be reverted if they occur).

See also the note in the linked docs which sounds good:

pyupgrade is intentionally timid and will not create an f-string if it would make the expression longer or if the substitution parameters are sufficiently complicated (as this can decrease readability).

I'm going to do the f-strings in a separate commit so you can have a look.

@StefRe StefRe force-pushed the mnt/python2 branch 3 times, most recently from 1b4dba3 to 7874ace Compare March 28, 2023 14:39
@StefRe StefRe marked this pull request as ready for review March 28, 2023 14:55
to improve readability and unify coding style (older code used %, newer
contributions used format etc). Modifications were done using pyupgrade.

Files in tinybuild\examples\future were deliberately left unchanged.
@StefRe
Copy link
Contributor Author

StefRe commented Mar 30, 2023

After rebasing I get two failing tests that pass locally so I assume it's not a real test failure. I can't "Rerun failed jobs" in https://dev.azure.com/sphinx-gallery/sphinx-gallery/_build/results?buildId=1246&view=results due to TF400813: The user '7dbe823d-48de-6791-9031-8f872678a096' is not authorized to access this resource. What should I do i such a case? Force push a dummy commit to retrigger the pipeline or is there something more intelligent?

@larsoner
Copy link
Contributor

I usually git commit --allow-empty -m 'TST: Ping' or something, yeah. (Arguably slightly cleaner than a force push.) But I do have permissions as an admin so I'll restart for you now :)

@larsoner larsoner merged commit 795e11b into sphinx-gallery:master Mar 30, 2023
15 checks passed
@larsoner
Copy link
Contributor

Thanks @StefRe !

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

Successfully merging this pull request may close these issues.

None yet

2 participants