Skip to content

MAINT: Use ruff-format instead of running black in pre-commit#16029

Merged
pllim merged 2 commits into
astropy:mainfrom
MridulS:ruff-format
Feb 26, 2024
Merged

MAINT: Use ruff-format instead of running black in pre-commit#16029
pllim merged 2 commits into
astropy:mainfrom
MridulS:ruff-format

Conversation

@MridulS
Copy link
Copy Markdown
Contributor

@MridulS MridulS commented Feb 13, 2024

This resolves #15757 as astropy/astropy-APEs#90 has been accepted.

c69079e has the changes caused by this.

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@github-actions
Copy link
Copy Markdown
Contributor

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • 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 instructions for rebase and squash.
  • 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 we cannot check for it on Actions; 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.

@neutrinoceros
Copy link
Copy Markdown
Contributor

I was curious to see how much this saved, so I looked at the recent history on pre-commit.ci
The run for this PR isn't hard to spot !
Screenshot 2024-02-13 at 12 48 07

Copy link
Copy Markdown
Contributor

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

I think it is customary to log large formatting commits in .git-blame-ignore-revs so they don't show up in git blame

@pllim
Copy link
Copy Markdown
Member

pllim commented Feb 13, 2024

We do have that file from when we initially implemented https://github.com/astropy/astropy-APEs/blob/main/APE20.rst but since then, we have not been adding follow-up ruff/black updates to it. Regardless, I don't think we can do this until we get the final merge commit after merging this PR? Anyways, if you want something added to https://github.com/astropy/astropy/blob/main/.git-blame-ignore-revs , either we do follow-up PR or I can do it directly on main as needed. Thanks!

@pllim pllim added this to the v6.1.0 milestone Feb 13, 2024
Copy link
Copy Markdown
Member

@eerovaher eerovaher left a comment

Choose a reason for hiding this comment

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

  1. #15998 should be merged before we start working on this.
  2. We have configured Black to ignore docs/ and examples/. I think it would be a good idea to start formatting Python code in those directories too, but not in this pull request.
  3. If we are not using Black anymore then all the Black configuration in pyproject.toml should be converted to equivalent Ruff formatter configuration.
  4. Our developer documentation needs to be updated.

@neutrinoceros
Copy link
Copy Markdown
Contributor

I don't think we can do this until we get the final merge commit after merging this PR?

Only if we squash merge, but as you said, it can be done in a follow up !

Copy link
Copy Markdown
Contributor

@WilliamJamieson WilliamJamieson left a comment

Choose a reason for hiding this comment

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

The modeling changes look like an actual improvement.

Copy link
Copy Markdown
Member

@larrybradley larrybradley left a comment

Choose a reason for hiding this comment

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

stats looks fine.

Copy link
Copy Markdown
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

Fine for time.

Copy link
Copy Markdown
Contributor

@mcara mcara left a comment

Choose a reason for hiding this comment

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

OK for WCS

Copy link
Copy Markdown
Member

@nstarman nstarman left a comment

Choose a reason for hiding this comment

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

cosmology & units both LGTM!
Also +1 to @eerovaher's list.

@MridulS
Copy link
Copy Markdown
Contributor Author

MridulS commented Feb 14, 2024

  1. We have configured Black to ignore docs/ and examples/. I think it would be a good idea to start formatting Python code in those directories too, but not in this pull request.

I will open a new PR for that. Copied over the black exclude list to ruff format too.

  1. If we are not using Black anymore then all the Black configuration in pyproject.toml should be converted to equivalent Ruff formatter configuration.

Done.

  1. Our developer documentation needs to be updated.

I have updated it in the places I saw black pop up. I have still put in links to the OG black style guide as ruff format wants to use that as the base and not deviate too much from it and the black docs explain it much better than ruff's docs.

Comment thread .ruff.toml Outdated
Comment thread astropy/time/formats.py Outdated
Comment thread astropy/time/formats.py Outdated
Comment thread astropy/utils/data.py Outdated
Comment thread astropy/utils/data.py Outdated
Comment thread pyproject.toml Outdated
Copy link
Copy Markdown
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Mostly changes for the better, except for two mathematics one. I think that should be reported upstream as undesirable.

Comment thread astropy/time/formats.py Outdated
Comment thread astropy/modeling/tests/test_models_quantities.py Outdated
Comment thread astropy/modeling/tests/test_models_quantities.py
Comment thread astropy/utils/tests/test_data.py Outdated
Comment thread pyproject.toml Outdated
Comment thread pyproject.toml Outdated
Comment on lines +363 to +365
# ISC001 shouldn't be used with ruff format
# https://docs.astral.sh/ruff/formatter/#conflicting-lint-rules
"ISC001",
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.

It's best to keep the list of ignored rules in alphabetical order. Admittedly RUF005 is in the wrong place too, but this pull request shouldn't worry about that (and I'm not convinced that RUF005 belongs on this list anyways).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done! Well I would argue as soon as we want to keep the things in order here, we should have a formatter tool :)
Time for pyproject-fmt?

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.

#15365 👀

Copy link
Copy Markdown
Member

@eerovaher eerovaher left a comment

Choose a reason for hiding this comment

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

This is now ready to be squashed and merged.

@pllim
Copy link
Copy Markdown
Member

pllim commented Feb 23, 2024

Not so fast, 3 warnings in doc build:

docs/development/codeguide.rst:575: WARNING: Duplicate explicit target name: "ruff".
docs/development/codeguide.rst:168: ERROR: Duplicate target name, cannot be used as a unique reference: "ruff".
docs/development/codeguide.rst:172: ERROR: Duplicate target name, cannot be used as a unique reference: "ruff".

Copy link
Copy Markdown
Member

@eerovaher eerovaher left a comment

Choose a reason for hiding this comment

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

The documentation build succeeds, so this really should be ready to be squashed and merged.

Copy link
Copy Markdown
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Thanks!

@pllim pllim merged commit af6c13c into astropy:main Feb 26, 2024
@pllim
Copy link
Copy Markdown
Member

pllim commented Feb 26, 2024

7 approvals! This must be a record. 😸

d-giles referenced this pull request in d-giles/astropy-abdufork Jul 26, 2024
* MAINT: Use ruff-format instead of running black in pre-commit

* use ruff link
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Use ruff format instead of black pre-commit hook?

10 participants