Skip to content

add pyupgrade to pre-commit - #13396

Merged
pllim merged 3 commits into
astropy:mainfrom
nstarman:pre-commit-pyupgrade
Jul 1, 2022
Merged

pllim merged 3 commits into
astropy:mainfrom
nstarman:pre-commit-pyupgrade

Conversation

@nstarman

@nstarman nstarman commented Jun 23, 2022 •

Copy link
Copy Markdown
Member

Pyupgrade upgrades syntax for newer versions of the language. Some features, like f-strings, are faster. Other features improve code style. All upgrades ensure code contributions meet the best practices of our minimum python version.
Also, when we bump a python version we can run pre-commit run —all-files to upgrade all existing code.

I also cleaned up the YAML with a YAML cleaner.

Signed-off-by: Nathaniel Starkman (@nstarman) [email protected]

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the Extra CI label.
  • Is a change log needed? If yes, did the change log check pass? If no, add the no-changelog-entry-needed label. If this is a manual backport, use the skip-changelog-checks label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate backport-X.Y.x label(s) before merge.

@nstarman nstarman added this to the v5.2 milestone Jun 23, 2022
@github-actions

Copy link
Copy Markdown
Contributor

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@mhvk

mhvk commented Jun 23, 2022

Copy link
Copy Markdown
Contributor

I found the changes elsewhere all pretty clear improvements, so would be happy to add this.

@nstarman

nstarman commented Jun 23, 2022 •

Copy link
Copy Markdown
Member Author

I'll rebase this PR when all the other ones are in. Hopefully it will pass / require very little further work.

@nstarman nstarman mentioned this pull request Jun 23, 2022
10 tasks
@WilliamJamieson

Copy link
Copy Markdown
Contributor

Do we want to exclude some directories from pyupgrade like astropy/extern?

@nstarman nstarman mentioned this pull request Jun 24, 2022
10 tasks done
@nstarman

Copy link
Copy Markdown
Member Author

Do we want to exclude some directories from pyupgrade like astropy/extern?

Yes, definitely a good idea.

Also, I'll ignore all the files in units and coordinates that are generated by extern/ply.

@nstarman
nstarman force-pushed the pre-commit-pyupgrade branch from 665a9b0 to c48cb73 Compare June 24, 2022 17:12
@nstarman
nstarman force-pushed the pre-commit-pyupgrade branch 2 times, most recently from 8cb4005 to c62f306 Compare June 30, 2022 19:46
@nstarman

nstarman commented Jul 1, 2022

Copy link
Copy Markdown
Member Author

I need to rebase to capture the timeseries related merge, but GTG otherwise

@nstarman
nstarman marked this pull request as ready for review July 1, 2022 00:40
nstarman added 3 commits July 1, 2022 11:55
and clean up the YAML with a YAML cleaner

Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman
nstarman force-pushed the pre-commit-pyupgrade branch from c62f306 to 9caaf11 Compare July 1, 2022 15:58
@nstarman

nstarman commented Jul 1, 2022

Copy link
Copy Markdown
Member Author

Rebased and ready for review. astropy/coordinates has a few changes. I don't think I ran pyupgrade with —py38-plus, when I pushed #13379.

@pllim pllim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't grok the pre-commit stuff so maybe @WilliamJamieson can look at that one. But the other changes LGTM. Thanks!

@WilliamJamieson WilliamJamieson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pre-commit looks fine to me, not sure about the rest of the changes.

@pllim
pllim merged commit 059adf9 into astropy:main Jul 1, 2022
@pllim

pllim commented Jul 1, 2022

Copy link
Copy Markdown
Member

Thanks!

@nstarman
nstarman deleted the pre-commit-pyupgrade branch July 1, 2022 19:14
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.

4 participants