Conversation
07c7639 to
949a3ee
Compare
CAM-Gerlach
left a comment
There was a problem hiding this comment.
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"} |
There was a problem hiding this comment.
| 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
| [tool.setuptools.dynamic] | ||
| version = {attr = "pyperf.__version__"} | ||
|
|
||
| [tool.setuptools] |
There was a problem hiding this comment.
| [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.)
There was a problem hiding this comment.
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.
|
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 |
|
I apply most of your suggestions, would you like to take a look before merging this PR?
Actually, it was intended after I met some messy cases, but I would like to try using the batch command as possible. |
|
Review in work now...
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 😆 ) |
|
Thank you @CAM-Gerlach @hugovk |
closes: #115