-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Move setup.cfg to pyproject.toml #15247
Conversation
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.
|
👋 Thank you for your draft pull request! Do you know that you can use |
b3f5edf
to
66c2655
Compare
0870b24
to
2d5e39f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🫰
b2afea1
to
69b21ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @WilliamJamieson! It’s great to migrate away from legacy config files. Aside from the changelog check not passing, let’s get this merged!
Would be nice if @astropy/astropy-project-release-team also get a chance to review. Let's wait a bit more. Thanks! |
Also, squash merge? |
I thought we did enforce 100 character line lengths? |
AFAIK that was entirely done by human review. I can't find anything in a quick perusal of the git history. |
Re: pep8speaks -- someone must have accidentally re-enabled it for this repo. I disabled it again but it is still enabled in the org for |
Back to the topic at hand, would be nice to get approval from @saimn since he had concerns before. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks, @WilliamJamieson!
.pycodestyle
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, see my comment on .flake8
.
@@ -6,7 +6,7 @@ | |||
import warnings | |||
|
|||
# First, the top-level packages: | |||
# TODO: This list is a duplicate of the dependencies in setup.cfg "all", but | |||
# TODO: This list is a duplicate of the dependencies in pyproject.toml "all", but | |||
# some of the package names are different from the pip-install name (e.g., | |||
# beautifulsoup4 -> bs4). | |||
_optional_deps = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a followup PR we could handle this more automatically if python>=3.11, using tomllib
& importlib.metadata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @WilliamJamieson for biting the bullet and leading this long-desired transition.
I have dug up contentious conversations from 2021 asking for cfg->toml.
0adc2df
to
66e18fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small comment, LGTM otherwise.
pyproject.toml
Outdated
authors = [ | ||
{ name = "The Astropy Developers", email = "astropy.team@gmail.com" } | ||
] | ||
license = { file = "LICENSE.rst" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to retain the SPDX identifier that was added some time ago.
license = { text = "BSD-3-Clause" }
Seems more informative. I'm not sure if we can put both in license
, and I cannot find a clear recommendation (https://peps.python.org/pep-0621/#license). We can also use setuptools' license-files
to specify the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, we cannot do both, per PEP621 (last sentence):
The table may have one of two keys. The file key has a string value that is a relative file path to the file which contains the license for the project. Tools MUST assume the file’s encoding is UTF-8. The text key has a string value which is the license of the project whose meaning is that of the License field from the core metadata. These keys are mutually exclusive, so a tool MUST raise an error if the metadata specifies both keys.
So I had to use the tool.setuptools.liscense-files
to specify the actual files, see 608019c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since @saimn 's last comment is addressed, I am going to squash and merge. Thanks, everyone!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed that, sounds good then, thanks @WilliamJamieson.
Also would be great to stop polluting PRs with unrelated discussions on config files and tools. The main concern here is packaging, and if a mistake slips in the noise we will have to deal with it when the release is out. |
Agreed we don't want to have a packaging problem! @saimn are you concerned about the failing weekly cron jobs? |
66e18fe
to
6e57164
Compare
Description
This pull request moves all the package configuration options from
setup.cfg
topyproject.toml
to bringastropy
inline with PEP621.