bpo-19610: Warn if distutils is provided something other than a list to some fields by nascheme · Pull Request #4685 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation20 Commits11 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
The commit of dcaed6b causes some 3rd party packages to fail to install, due to them passing a tuple for meta data fields. Rather than raise TypeError, generate a RuntimeWarning and convert the value using list().
https://bugs.python.org/issue19610
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed to update Doc/distutils/apiref.rst too.
| @@ -354,8 +354,11 @@ def test_classifier_invalid_type(self): |
|---|
| attrs = {'name': 'Boa', 'version': '3.0', |
| 'classifiers': ('Programming Language :: Python :: 3',)} |
| msg = "'classifiers' should be a 'list', not 'tuple'" |
| with self.assertRaises(TypeError, msg=msg): |
| Distribution(attrs) |
| with self.assertWarns(RuntimeWarning) as warns: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would is better to use assertWarnsRegex() here and in other tests. Otherwise msg isn't used.
| Distribution(attrs) |
|---|
| with self.assertWarns(RuntimeWarning) as warns: |
| d = Distribution(attrs) |
| self.assertIsInstance(d.metadata.classifiers, list) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be better to move these checks outside of the with block. Otherwise it isn't clear what operation raises a RuntimeWarning.
| import warnings |
|---|
| except ImportError: |
| warnings = None |
| import warnings |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s possible that the import was how it was because distutils is used to build CPython’s own extension modules. In doubt, let’s not change this.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that distutils also has its own PEP-282-style logging system, which may or may not easier to use here. The output is controlled by verbose/quiet options passed to setup.py rather than the interpreter warning configuration system, so testing with pip would be needed to make sure that package authors would see the messages by default.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The try/except dates back to Python 1.5 + 2.0 days. It was added because those versions didn't have 'warnings' and distutils was targeting those versions as well. I think removing it is safe but I will revert as it is a separate change from this issue.
Using the logging system sounds like a good idea. I'll look into that.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made the requested changes; please review again.
I tweaked the wording of the warning message, not sure it is the best.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch. Tests look good; left comment about conditional use of warnings.
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
While we a here could you please add value = list(value) in set_requires() and set_obsoletes(). They already work with arbitrary iterables, but this can have unexpected effect in upload.py and register.py.
I have made the requested changes; please review again.
Thanks for making the requested changes!
@serhiy-storchaka, @merwok: please review the changes made to this pull request.
| warnings.warn(msg) |
|---|
| else: |
| sys.stderr.write(msg + "\n") |
| warnings.warn(msg) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought you wanted to move this in a separate PR?
| elif not isinstance(value, list): |
|---|
| # passing a tuple or an iterator perhaps, warn and convert |
| typename = type(value).__name__ |
| msg = f"Warning: '{fieldname}' should be a list, got type '{typename}'" |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the prefix Warning redundant? Otherwise message seems good.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another style nit: Can't we use {fieldname!r} instead of '{fieldname}'?
| @@ -26,6 +26,18 @@ |
|---|
| # to look for a Python module named after the command. |
| command_re = re.compile(r'^[a-zA-Z]([a-zA-Z0-9_]*)$') |
| def _ensure_list(value, fieldname): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: would you mind keeping two blank lines before and after this function?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good to me, thank you! I just left some comments on code style.
| @@ -11,7 +11,8 @@ |
|---|
| from distutils.dist import Distribution, fix_help_options, DistributionMetadata |
| from distutils.cmd import Command |
| from test.support import TESTFN, captured_stdout, run_unittest |
| from test.support import TESTFN, captured_stdout, captured_stderr, \ |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: Replacing \ with
from test.support import ( TESTFN, captured_stdout, captured_stderr, run_unittest, )
may be better.
| @@ -1231,7 +1223,7 @@ def set_requires(self, value): |
|---|
| import distutils.versionpredicate |
| for v in value: |
| distutils.versionpredicate.VersionPredicate(v) |
| self.requires = value |
| self.requires = list(value) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| elif not isinstance(value, list): |
|---|
| # passing a tuple or an iterator perhaps, warn and convert |
| typename = type(value).__name__ |
| msg = f"Warning: '{fieldname}' should be a list, got type '{typename}'" |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another style nit: Can't we use {fieldname!r} instead of '{fieldname}'?
| msg = "'platforms' should be a 'list', not 'tuple'" |
|---|
| with self.assertRaises(TypeError, msg=msg): |
| Distribution(attrs) |
| msg = "should be a list" |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: I think we can now inline this in line 395:
self.assertIn(msg, error.getvalue())
Introduce a helper function _ensure_list() that reduces code duplication.
Labels
An unexpected behavior, bug, or error