Skip to content

gh-115: Migrate to the pyproject.toml #149

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

Merged
merged 7 commits into from
Nov 28, 2022
Merged

gh-115: Migrate to the pyproject.toml #149

merged 7 commits into from
Nov 28, 2022

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Nov 27, 2022

closes: #115

@corona10 corona10 requested a review from vstinner November 27, 2022 11:31
@corona10 corona10 force-pushed the gh-115 branch 3 times, most recently from 07c7639 to 949a3ee Compare November 27, 2022 12:58
Copy link

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Not 100% comprehensive, but this should fix the most important issues I noticed looking through it

pyproject.toml Outdated
[project]
name = "pyperf"
dynamic = ["version"]
license = {text = "MIT license"}

Choose a reason for hiding this comment

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

Suggested change
license = {text = "MIT license"}
license = {text = "MIT"}

PEP 639 is still in work, but in the meantime its worth still using standard SPDX identifiers here, and repeating "license" is rather redundant given this is the "license" field, after all

pyproject.toml Outdated
Comment on lines 52 to 55
[tool.setuptools.dynamic]
version = {attr = "pyperf.__version__"}

[tool.setuptools]

Choose a reason for hiding this comment

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

Suggested change
[tool.setuptools.dynamic]
version = {attr = "pyperf.__version__"}
[tool.setuptools]
[tool.setuptools]

If you specify the version statically above, this section can be eliminated. (It also avoids these sections being out of order.)

Copy link
Member Author

Choose a reason for hiding this comment

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

This was intended, I would like to reduce the defining multiple version semantics.
Currently, we have 3 sections about versioning.

Do you have better ideas?

Choose a reason for hiding this comment

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

I understand; I just wanted to mention that there is some amount of cost to it. But there's nothing wrong per-say about the current approach; if you're sure that's what you want, what you have would be the way to do it using base Setuptools.

I do suggest, though, moving the tool.setuptools.dynamic section below the tool.setuptools section to follow standard TOML conventions.

To avoid the duplication in the conf.py, since its a dynamic Python file, you can read the version from the __init__.py via any number of means—unfortunately, as it imports a bunch of stuff, best not to use exec or importlib, but you could extract it with re, ast or other methods.

A popular option for further single-sourcing both the version and other things is Setuptools_scm; the amount of single-sourcing is higher, but also the potential cost and complexities for some types of packaging scenarios. So, likely worth considering separately from this PR.

@CAM-Gerlach
Copy link

CAM-Gerlach commented Nov 27, 2022

As a general tip that even experienced folks often don't remember or aren't aware of, you can easily apply some or all suggestions directly in one go instead of repeating the same changes yourself manually, which can be time-wasting, tedious and error prone (as was the case here), by going to the Files changed tab, clicking Add to batch on each suggestion, and then clicking Commit to commit them all in one go. If you want to modify a suggestion or make your own, you can also easily do so in your own reply comment and add/apply that alongside the others. It also auto-resolves the relevant suggestions, clearly marks the relevant commits with the reviewer(s) whose suggestions were applied and can reduce the need for reviewers to manually recheck the changes, which makes things easier for reviewers, too.

@corona10 corona10 requested review from hugovk and CAM-Gerlach and removed request for hugovk and CAM-Gerlach November 27, 2022 14:00
@corona10
Copy link
Member Author

@hugovk @CAM-Gerlach

I apply most of your suggestions, would you like to take a look before merging this PR?

suggestions directly in one go instead of repeating the same changes yourself manually

Actually, it was intended after I met some messy cases, but I would like to try using the batch command as possible.
Thanks for the suggestion.

@CAM-Gerlach
Copy link

Review in work now...

Actually, it was intended after I met some messy cases, but I would like to try using the batch command as possible.

Yeah, totally fine of course—I just know a lot of folks, even core devs, don't always know or remember this feature and as a reviewer I feel guilty when others, especially core devs like you, spend their valuable time implementing my suggestions. The other advantage for authors I didn't mention is that if something breaks with an applied suggestion, you know its the reviewer that messed up (as I all to often do 😆 )

Copy link

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @corona10 👍

@corona10 corona10 merged commit 7189db4 into psf:main Nov 28, 2022
@corona10
Copy link
Member Author

Thank you @CAM-Gerlach @hugovk
If there needs more improvement please let me know through the issue tracker :)

@corona10 corona10 deleted the gh-115 branch November 28, 2022 04:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to the pyproject.toml
3 participants