Skip to content

Enable the use of pyproject-fmt#15365

Closed
WilliamJamieson wants to merge 1 commit into
astropy:mainfrom
WilliamJamieson:feature/pyproject_fmt
Closed

Enable the use of pyproject-fmt#15365
WilliamJamieson wants to merge 1 commit into
astropy:mainfrom
WilliamJamieson:feature/pyproject_fmt

Conversation

@WilliamJamieson
Copy link
Copy Markdown
Contributor

@WilliamJamieson WilliamJamieson commented Sep 21, 2023

Description

Now that #15247 has been merged we may want to consider standardizing the formatting of our pyproject.toml file. As noted in #15247 (comment) the pyproject-fmt tool has been created by the tox devs in order to apply consistent formatting to to their pyproject.toml files across their various projects.

This PR is applies the pyproject-fmt tool to astropy and enables its continued use via our pre-commit.

  • 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.

@WilliamJamieson WilliamJamieson added no-changelog-entry-needed Build all wheels Run all the wheel builds rather than just a selection labels Sep 21, 2023
@pllim pllim added this to the v6.0 milestone Sep 21, 2023
@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.

@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?

Comment thread pyproject.toml Outdated
@WilliamJamieson
Copy link
Copy Markdown
Contributor Author

I am opening this PR so that we can discuss the pros/cons of using this.

@nstarman
Copy link
Copy Markdown
Member

nstarman commented Sep 21, 2023

In future I'd like to only think about the contents of this file, not how it's formatted, so I'm in favor of this PR.
Automate away.

Comment thread .pre-commit-config.yaml
rev: "1.0.0"
hooks:
- id: pyproject-fmt
args: ["--indent=4"]
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.

Unfortunately, I cannot figure out how to independently configure the indent setting (requested by @pllim) outside of specifying it via command line args. I have opened tox-dev/pyproject-fmt#130 for assistance with this.

@WilliamJamieson WilliamJamieson marked this pull request as ready for review September 21, 2023 17:02
Comment thread pyproject.toml
"Programming Language :: Python :: 3 :: Only",
"Programming Language :: Python :: 3.9",
"Programming Language :: Python :: 3.10",
"Programming Language :: Python :: 3.11",
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.

Will this tool auto-add python3.12?

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.

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.

Now this is nice

@pllim
Copy link
Copy Markdown
Member

pllim commented Sep 21, 2023

Maybe put this in the next dev telecon agenda? I am neutral and I am interested in what other devs think.

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.

Somewhat mixed about it changing ordering in what seems an arbitrary fashion. I'd like the first astropy keyword to be "astronomy", not "ascii".

p.s. I find pre-commit a bit of a pain in slowing down development; this presumably is very fast if pyproject.toml is not touched?

Comment thread pyproject.toml
"tomli; python_version < '3.11'",
'tomli; python_version < "3.11"',
]
recommended = [
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.

Order again had (some) meaning here. That said, in this case I can see the advantage of just being alphabetical.

Comment thread pyproject.toml
[tool.ruff.pydocstyle]
convention = "numpy"

[tool.pytest.ini_options]
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.

This I don't understand; either be alphabetical or not - why is this moved past tools.ruff? Certainly, pytest is far more important to see than ruff.

Comment thread pyproject.toml
]
description = "Astronomy and astrophysics core library"
readme = "README.rst"
keywords = [
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 alphabetization has removed meaning here. astropy is not first and foremost about ascii (not sure why that was put on there, though I guess table reading...).

@nstarman
Copy link
Copy Markdown
Member

p.s. I find pre-commit a bit of a pain in slowing down development; this presumably is very fast if pyproject.toml is not touched?

Interesting. What step in pre-commit is slow?

@WilliamJamieson
Copy link
Copy Markdown
Contributor Author

The remove star is slow.

@nstarman
Copy link
Copy Markdown
Member

On run or only run -a?

@nstarman
Copy link
Copy Markdown
Member

I find it a bit slow on run -a, but not run (unless I've edited every file in GH, of course)

@nstarman
Copy link
Copy Markdown
Member

Ping @Saransh-cpp for this discussion of speed.

@mhvk
Copy link
Copy Markdown
Contributor

mhvk commented Sep 22, 2023

It is the collection. And, no, it is not that slow, it is just enough to be irritating, that's why I'm a bit wary of adding more steps. (Plus I fairly regularly clean up the repository with git clean -fxd, and then one has to reinstall, but that's arguably on me.)

@nstarman
Copy link
Copy Markdown
Member

It is the collection. And, no, it is not that slow, it is just enough to be irritating

Interesting. I'm on an M2 MBPro and it's fast enough I don't notice.
I wonder what steps are slow, maybe black & removestar? They have to build an AST in Python. The rest are regex or rust

@WilliamJamieson
Copy link
Copy Markdown
Contributor Author

removestar is noticeably slow for me.

@nstarman
Copy link
Copy Markdown
Member

We could move removestar to a cron job, or if asmeurer/removestar#51 (or similar) can speed it up...

@Saransh-cpp
Copy link
Copy Markdown
Member

Thanks for the ping!

Re pyproject-fmt:

The hook was not added to scientific-python's development guide because it was not very flexible - scikit-hep/scikit-hep.github.io#214 (scikit-hep.github.io was ported to create learn.scientific-python.org/development). I've also discovered some bugs in the tool in the past, and though I planned to work on them, I never got the time :(

(not completely against the hook, some of my repositories use it, some of them don't, and I really appreciate the work done on it, everything depends on how well it fits within astropy!)

Re removestar's speed:

I can shift it to the cron job until I profile and identify the bottleneck. This might take some time, and I am not sure when I'll start working on it 🙂

@saimn saimn modified the milestones: v6.0, v6.1 Nov 3, 2023
@github-actions github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label Feb 19, 2024
@github-actions
Copy link
Copy Markdown
Contributor

Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

@github-actions
Copy link
Copy Markdown
Contributor

I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks!

If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here.

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

Labels

Build all wheels Run all the wheel builds rather than just a selection Close? Tell stale bot that this issue/PR is stale closed-by-bot Closed by stale bot dev-automation installation no-changelog-entry-needed skip-changelog-checks Tells bot to skip changlog checks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants