Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
WIP: updated single_source_version with a much simpler page. #1578
WIP: updated single_source_version with a much simpler page. #1578
Changes from 10 commits
fb3ec88
c7fa00c
49842a8
840801f
3724c8d
5368956
56db0d9
9bace5d
035c2bd
dd1b70e
eaf458a
29aa220
de722f6
63061bd
bfdc474
c69e2c0
ddb077c
648c427
b9ced45
0f5d2d3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I feel like this introduced a lot of controversy in the past. The way it's written here, it reads as though the “
__version__
” name specifically must be used and nothing else. The reality is that the variable name can be arbitrary. And it doesn't have to be top-level to work, either. Additionally, I see that here and in the previous point, the word “package” is used, but it means different things. Here, you talk about an importable, while there it's about a distribution. Can we do anything about this?Could you try rephrasing this to say that an in-project importable module may have a hard-coded variable that can be wired into packaging metadata by name? We can mention that many projects use
__version__
for the variable name, but there is no standard mandating it to be that.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.
I suggested an edit in response to your comment.
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.
Additionally, we should maybe mention that
__version__
may be populated fromimportlib.metadata.version(__name__)
to provide the version string from installed packaging metadata as an importable.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.
(ref #1276 (comment))
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.
As near as I can tell, the push to get rid of
__version__
came from people protesting to this pattern (as a waste of runtime on import).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.
Yeah, I'm starting to understand this better now, having read another thread: #1578 (comment)
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.
It also comes from the fact that
__version__
is just a conventional place to duplicate the information in that already has a canonical place: not very pythonicThere 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.
I respectfully, but completely disagree with this characterization.
__version__
is the (defacto) standard which predates the existence of pip by at least 7 years (in Matplotlib I have a commit from 2004 using__version__
) let alone the current push to bootstrap another (Python -specific) packaging ecosystem.From my perspective the "duplicate" here is
dist-info
and as I noted on discourse I am not worried about this. Package managers (apt/dpkg, yum, pacman, conda, pip, ....) all need their own copy of the version metadata (and dependencies) to do their jobs. This is OK and expected as each of these systems, and runtime, have different needs and hence (may) need their own copy of the information.It comes across as very hostile to invent a new place to put the version information and then declare the existing location an upythonic duplicate!
As a thought experiment, if I said "We can scrap all of
dist-info
, we can query pacman to get all of it when we need it" I don't think it would be taken seriously. "just use the metadata for pip" reads exactly the same to me.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.
@flying-sheep I think that it's a bit more complicated than that. Duplicating the hardcoded values is bad. But if only one place is the actual source of the information and another just reflects the same information though a different API, that should be okay.
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.
@tacaswell TBF, from the Python packaging perspective, that is not “metadata for pip” by a standardized machine-readable information. That said, if a project is meant to be installed and the installed dist metadata contains a version, it's reasonable to threat it as the source for the most accurate information (even if it originated from some importables that could be accessed separately). From that perspective, it might be the source of truth in many contexts.
But it's rather tricky to explain/document this in an accessible manner. Hence, my suggestion to attempt visualizing these relationships with my diagram #1276 (comment) (or something close to it).
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 should be one – and preferably only one – way to do it
the preferable way is the standard way
__version__
is older, but it’s not standardized in a PEP, so it’s not the standard.also putting that info into an import package doesn’t make sense, as the versioned unit is the distribution package – i.e. the thing that has the
dist-info
directory in it that contains the standard metadata. most distribution packages might come with one main import package (e.g.Pillow
contains the import packagePIL
), but that’s not a rule, so the only place that will reliably match 1:1 with versioned units is the distribution package, complete with its 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.
It looks like other references use spaces. Perhaps, follow the same style?