-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
bpo-19610: Warn if distutils is provided something other than a list to some fields #4685
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
Changes from all commits
2459267
a440128
4a7bfe3
8024d47
8a4c09e
eeecfdd
76a1cec
d27b625
1f79d9f
e31669a
67503b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,20 @@ | |
command_re = re.compile(r'^[a-zA-Z]([a-zA-Z0-9_]*)$') | ||
|
||
|
||
def _ensure_list(value, fieldname): | ||
if isinstance(value, str): | ||
# a string containing comma separated values is okay. It will | ||
# be converted to a list by Distribution.finalize_options(). | ||
pass | ||
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}'" | ||
log.log(log.WARN, msg) | ||
value = list(value) | ||
return value | ||
|
||
|
||
class Distribution: | ||
"""The core of the Distutils. Most of the work hiding behind 'setup' | ||
is really done within a Distribution instance, which farms the work out | ||
|
@@ -257,10 +271,7 @@ def __init__(self, attrs=None): | |
setattr(self, key, val) | ||
else: | ||
msg = "Unknown distribution option: %s" % repr(key) | ||
if warnings is not None: | ||
warnings.warn(msg) | ||
else: | ||
sys.stderr.write(msg + "\n") | ||
warnings.warn(msg) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought you wanted to move this in a separate PR? |
||
|
||
# no-user-cfg is handled before other command line args | ||
# because other args override the config files, and this | ||
|
@@ -1189,36 +1200,19 @@ def get_keywords(self): | |
return self.keywords or [] | ||
|
||
def set_keywords(self, value): | ||
# If 'keywords' is a string, it will be converted to a list | ||
# by Distribution.finalize_options(). To maintain backwards | ||
# compatibility, do not raise an exception if 'keywords' is | ||
# a string. | ||
if not isinstance(value, (list, str)): | ||
msg = "'keywords' should be a 'list', not %r" | ||
raise TypeError(msg % type(value).__name__) | ||
self.keywords = value | ||
self.keywords = _ensure_list(value, 'keywords') | ||
|
||
def get_platforms(self): | ||
return self.platforms or ["UNKNOWN"] | ||
|
||
def set_platforms(self, value): | ||
# If 'platforms' is a string, it will be converted to a list | ||
# by Distribution.finalize_options(). To maintain backwards | ||
# compatibility, do not raise an exception if 'platforms' is | ||
# a string. | ||
if not isinstance(value, (list, str)): | ||
msg = "'platforms' should be a 'list', not %r" | ||
raise TypeError(msg % type(value).__name__) | ||
self.platforms = value | ||
self.platforms = _ensure_list(value, 'platforms') | ||
|
||
def get_classifiers(self): | ||
return self.classifiers or [] | ||
|
||
def set_classifiers(self, value): | ||
if not isinstance(value, list): | ||
msg = "'classifiers' should be a 'list', not %r" | ||
raise TypeError(msg % type(value).__name__) | ||
self.classifiers = value | ||
self.classifiers = _ensure_list(value, 'classifiers') | ||
|
||
def get_download_url(self): | ||
return self.download_url or "UNKNOWN" | ||
|
@@ -1231,7 +1225,7 @@ def set_requires(self, value): | |
import distutils.versionpredicate | ||
for v in value: | ||
distutils.versionpredicate.VersionPredicate(v) | ||
self.requires = value | ||
self.requires = list(value) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add tests for the requires and obsolotes fields similar to what you already added for the classifiers field?: attrs = {...,
'classifiers': ('Programming Language :: Python :: 3',)}
self.assertIsInstance(d.metadata.classifiers, list)
self.assertEqual(d.metadata.classifiers,
list(attrs['classifiers'])) If I don't miss something, the line https://github.com/python/cpython/blob/bce095d9b0595a528a95bae3f764ee195a0c0d63/Lib/distutils/tests/test_dist.py#L301-L315 |
||
|
||
def get_provides(self): | ||
return self.provides or [] | ||
|
@@ -1250,7 +1244,7 @@ def set_obsoletes(self, value): | |
import distutils.versionpredicate | ||
for v in value: | ||
distutils.versionpredicate.VersionPredicate(v) | ||
self.obsoletes = value | ||
self.obsoletes = list(value) | ||
|
||
def fix_help_options(options): | ||
"""Convert a 4-tuple 'help_options' list as found in various command | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,4 @@ | ||
``setup()`` now raises :exc:`TypeError` for invalid types. | ||
``setup()`` now warns about invalid types for some fields. | ||
|
||
The ``distutils.dist.Distribution`` class now explicitly raises an exception | ||
when ``classifiers``, ``keywords`` and ``platforms`` fields are not | ||
specified as a list. | ||
The ``distutils.dist.Distribution`` class now warns when ``classifiers``, | ||
``keywords`` and ``platforms`` fields are not specified as a list or a string. |
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.
Is the prefix
Warning
redundant? Otherwise message seems good.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.
Another style nit: Can't we use
{fieldname!r}
instead of'{fieldname}'
?