-
-
Notifications
You must be signed in to change notification settings - Fork 31k
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
bpo-35186: Remove "built with" comment in setup.py upload #10414
Conversation
@pganssle Yes, the field is "used" in the sense that it's accepted and stored by PyPI. It's an optional, free-form field used to add comments to distributions. The comments are available in the API and appear as tooltips for the files in PyPI's management UI: The format or contents of the comment here is not specifically used for anything in PyPI, though. |
I think as long as it's only used as a free-form comment it should be fine to remove it from At worst this is going to break people who are parsing the comment field from PyPI for whatever reason, and I suspect that's both exceedingly rare and not really something anyone is eager to support. |
Yea, what @di said. It has no semantic meaning, just a freeform whatever field. Honestly we might even want to consider removing it, I'm not sure it really makes much sense in the modern world where almost all downloads from PyPI are automated tooling and not humans making project by project decisions on what file to download. |
platform.dist() is deprecated and slated for removal in Python 3.8. The upload command itself should also not be used to upload to PyPI, but while it continues to exist it should not use deprecated functions. Fixes bpo-35186
a74a6e5
to
27153b8
Compare
Ok, it looks like we're leaning toward removing the comment, so I've added a news entry to that effect. |
This comment is not used anywhere and `platform.dist()` is deprecated. See CPython PR #10414: python/cpython#10414 and bpo-35186: https://bugs.python.org/issue35186
@berkerpeksag @encukou @malemburg Are y'all the ones who are moving the deprecation of |
comment = 'built for %s' % platform.platform(terse=1) | ||
data['comment'] = comment | ||
|
||
data['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.
Does it make a difference to send empty string or omit comment altogether?
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.
The old version sent an empty string by default, so I kept that behavior, but I just checked what twine
does, and it does seem to send ''
even when comment=None
(the default), so I think that distutils
should do the same.
I can't see how this comment is useful (esp. if it's added automatically), so I'd be for removing it. |
@encukou @berkerpeksag @merwok Anyone want to push the merge button? Just a ping, let me know if there are any other concerns before merging. |
@encukou Not sure if your last message means «+1 on this PR» or «please change something in the PR» |
A +1. Thanks for the reminder! |
platform.dist() is deprecated and slated for removal in Python 3.8. The upload command itself should also not be used to upload to PyPI, but while it continues to exist it should not use deprecated functions.
This is the first option I proposed in bpo-35186. A slightly more conservative option (option 2) is easy enough to implement:
Distutils maintainers let me know how you want to go. I'll hold off on the news entry until after we decide which way to go.
https://bugs.python.org/issue35186