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

Change flake8 to ruff #14109

Merged
merged 7 commits into from
Dec 23, 2022
Merged

Change flake8 to ruff #14109

merged 7 commits into from
Dec 23, 2022

Conversation

nstarman
Copy link
Member

@nstarman nstarman commented Dec 6, 2022

Description

This PR transitions us from flake8 to ruff: a new-but-already-well-established tool for linting. See https://github.com/charliermarsh/ruff for details. In short, ruff is really fast. It's easier for the CI and for IDE. I and others (@WilliamJamieson) have been using Ruff for a while and I can attest that, on personal projects using ruff, when I make a change in one part of the codebase ruff has finished checking for inconsistencies throughout the rest of my code by the time I've lifted my finger off of the save shortcut. I highly recommend this quality of life improvement for Astropy.

TLDR:
Replacing flake8 with ruff, which is over 2 orders of magnitude faster and has a lot of auto-fix capabilities.
Ruff is at near feature parity with base flake8, builds in many plugins for native support, and did I mention it's REALLY fast?

Q: What happened to the F811?

Ruff doesn't support that yet / ever, so those noqa are removed.

Ruff now support F811, bringing it to full flake8 parity (assuming isort and black are used).

Q: Why get rid of yesqa?

Ruff replaces it! the RUF100 code means ruff will check if a noqa is necessary.

Q: What's happening with the .html stuff in cosmology?

There were weird ascii issues that ruff is correcting.

Q: What's happening with the F401?

Ruff changes a noqa F401 that applies to a block of imports to individual import ignores. Overall, this is an improvement as we can better see which import requires the special treatment.

Q: Does ruff integrate with my IDE?

See https://github.com/charliermarsh/ruff#editor-integrations!
If it doesn't integrate natively, ruff also has a watch mode that reruns on any change (https://github.com/charliermarsh/ruff#usage). If you're thinking that this will take too long to run on every save, allow me to welcome you out of flake8 land, where your computer fan is constantly on max, into this wonderful Eden of ruff.

Q: Are ruff's settings configured on pyproject.toml?

😭. Yes! it's soooo nice to have a linter gladly adopt the future of config files. While I personally like the syntax of setup.cfg more than pyproject.toml, standardization and unification across the ecosystem is vastly more important.

Approvals:
constants: βœ…
convolution: βœ…
coordinates: βœ…
cosmology: βœ…
io.ascii: βœ…
io.fits:
io.misc.asdf: βœ…
nddata:
table: βœ…
time: βœ…
units: βœ…
visualization: βœ…
wcs:

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. Codestyle issues can be fixed by the bot.
  • 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
Copy link
Member Author

nstarman commented Dec 9, 2022

Also, ruff now supports F811, so I need to rebase and make sure the PR reflect that. It's very good to supports F811, that can prevent hard-to-track-down bugs.

@WilliamJamieson
Copy link
Contributor

@nstarman I just noticed that the pre-commit.ci bot was not updating the pin for the ruff pre-commit entry for asdf, see https://github.com/asdf-format/asdf/blob/32b82b8e3e57f7a3ae631dad0ff452bf11cbe8bb/.pre-commit-config.yaml#L58. So we might want to keep that in mind.

@WilliamJamieson
Copy link
Contributor

@nstarman I just noticed that the pre-commit.ci bot was not updating the pin for the ruff pre-commit entry for asdf, see asdf-format/asdf@32b82b8/.pre-commit-config.yaml#L58. So we might want to keep that in mind.

It maybe because they switched from the keyword lint to ruff for the job id... So it would fail to install properly with an update.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@nstarman
Copy link
Member Author

nstarman commented Dec 22, 2022

In a followup PR, should we enforce E501? Black ensures it applies to code, but it does not apply to docstrings, comments, nor long strings. In #14127 we eliminated the E501 check because, though it was in the setup.cfg configuration, it wasn't enforced in pre-commit, instead relying on maintainers to enforce.

@WilliamJamieson
Copy link
Contributor

In a followup PR, should we enforce E501? Black ensures it applies to code, but it does not apply to docstrings, comments, nor long strings. In #14127 we eliminated the E501 check because, though it was in the setup.cfg configuration, it wasn't enforced in pre-commit, instead relying on maintainers to enforce.

How many errors do we get if we check E501?

@nstarman
Copy link
Member Author

nstarman commented Dec 22, 2022

How many errors do we get if we check E501?

A LOT (In a quick test branch I found 184 changed files with 876 additions and 853 deletions). Sadly many of the fixes will be manual. Many comments are easily fixed by VSCode and other IDEs' hard wrap functionalities. Also the docstrings are mostly fixed the same way, with the exception of parameter type descriptions.

Parameters
β€”β€”β€”β€”β€”β€”
arg : if this is super long there's no good way to shorten it.

For many of the rest, we have an opportunity to improve the code, e.g. in ``wcs/wcs-api/utils.py`:

        s += (('{0:' + str(pixel_dim_width) + 'g}').format(ipix) + '  ' +
                ('{0:' + str(pixel_nam_width) + 's}').format(wcs.pixel_axis_names[ipix] or 'None') + '  ' +
                (" " * 5 + str(None) if pixel_shape[ipix] is None else
                ('{0:' + str(pixel_siz_width) + 'g}').format(pixel_shape[ipix])) + '  ' +
                '{:s}'.format(str(None if wcs.pixel_bounds is None else wcs.pixel_bounds[ipix]) + '\n'))

can be rewritten using nested f-strings as

        s += (
            f"{ipix:{pixel_dim_width}g}  "
            + f"{(wcs.pixel_axis_names[ipix] or 'None'):{pixel_nam_width}s}  "
            + ("     None  " if pixel_shape[ipix] is None
               else f"{pixel_shape[ipix]:{pixel_siz_width}g}  ")
            + "None\n" if wcs.pixel_bounds is None else f"{wcs.pixel_bounds[ipix]:s}\n")

@pllim
Copy link
Member

pllim commented Dec 22, 2022

tox keeps pumping out bug fix, so maybe we don't have to wait for tox 4 compat.

setup_function,
teardown_function,
)
from .common import setup_function # noqa: F401
Copy link
Member

Choose a reason for hiding this comment

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

tl;dr -- Is this undoing black changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorta β€” I think more undoing an isort change. ruff sits on top of isort and black. The former aggregated these imports, the latter separated them over multiple lines. ruff assumes a project uses both isort and black and adds this further style refinement, disaggregating imports with "errors".

@pllim
Copy link
Member

pllim commented Dec 22, 2022

A LOT

Maybe hold off on E501 for now? 😬

@nstarman
Copy link
Member Author

Yeah, definitely for this PR!

@nstarman
Copy link
Member Author

tox keeps pumping out bug fix, so maybe we don't have to wait for tox 4 compat.

πŸŽ‰ ... I think we have a critical mass of approvals ?...

@pllim

This comment was marked as resolved.

@pllim
Copy link
Member

pllim commented Dec 23, 2022

OK CI should be green now in main. Please rebase. Thanks!

Would be nice to know this didn't accidentally break anything.

nstarman and others added 7 commits December 22, 2022 23:17
Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>
Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>
Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>
Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>
Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>
Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>
Co-authored-by: William Jamieson <wjamieson@stsci.edu>
@nstarman
Copy link
Member Author

πŸŽ‰

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

8 participants