Skip to content

bpo-19610: setup() now raises TypeError for invalid types#4519

Merged
berkerpeksag merged 2 commits into
python:masterfrom
berkerpeksag:19610-distutils
Nov 23, 2017
Merged

bpo-19610: setup() now raises TypeError for invalid types#4519
berkerpeksag merged 2 commits into
python:masterfrom
berkerpeksag:19610-distutils

Conversation

@berkerpeksag

@berkerpeksag berkerpeksag commented Nov 23, 2017 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

The Distribution class now explicitly raises an
exception when 'classifiers', 'keywords' and
'platforms' fields are not specified as a list.

https://bugs.python.org/issue19610

@berkerpeksag
bpo-19610: setup() now raises TypeError for invalid types
5cf0e6f 
The Distribution class now explicitly raises an
exception when 'classifiers', 'keywords' and
'platforms' fields are not specified as a list.
@berkerpeksag berkerpeksag requested a review from merwok November 23, 2017 13:12
@cryvate

cryvate commented Nov 23, 2017

Copy link
Copy Markdown
Contributor

LGTM code + docs.

Is there a test that qualifiers cannot be a string? Might be a good way to make sure this isn't accidentally added, contradicting the docs.

@berkerpeksag

berkerpeksag commented Nov 23, 2017

Copy link
Copy Markdown
Member Author

Quoting @cryvate:

Is there a test that qualifiers cannot be a string? Might be a good way to make sure this isn't accidentally added, contradicting the docs.

Do you mean is there a test for fields other than keywords, platforms and classifiers? Distribution.finalize_options() only converts a string to a list for keywords and platforms fields:

cpython/Lib/distutils/dist.py

Lines 602 to 608 in b98f171

for attr in ('keywords', 'platforms'):
value = getattr(self.metadata, attr)
if value is None:
continue
if isinstance(value, str):
value = [elm.strip() for elm in value.split(',')]
setattr(self.metadata, attr, value)

@cryvate

cryvate commented Nov 23, 2017 via email •
edited by merwok
Loading

Copy link
Copy Markdown
Contributor

@berkerpeksag

berkerpeksag commented Nov 23, 2017

Copy link
Copy Markdown
Member Author

There is no "string to list" test but "tuple to list" test for classifiers: test_classifier_invalid_type

I think that covers the following branch:

if not isinstance(value, list):
    ...

And since the implicit "string to list" conversation only happens for 'keywords' and 'platforms' fields, I think we're good to go.

Please feel free to send another PR or an example if I misunderstand your comment, thanks!

@berkerpeksag berkerpeksag merged commit dcaed6b into python:master Nov 23, 2017
@berkerpeksag berkerpeksag deleted the 19610-distutils branch November 23, 2017 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merwok merwok merwok approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@berkerpeksag @cryvate @merwok @the-knights-who-say-ni @bedevere-bot