Disallow files for license inputs #1559
Conversation
setuptools/config.py
Outdated
| def _exclude_files_parser(cls, value): | ||
| exclude_directive = 'file:' | ||
| if value.startswith(exclude_directive): | ||
| raise ValueError('Only strings are accepted, Files are not accepted') |
There was a problem hiding this comment.
I think this should be files instead of Files, but I'm a bit worried that this will be a confusing error message if we can't tell them the field that caused the problem.
setuptools/tests/test_egg_info.py
Outdated
| build_files({'setup.py': setup_script, | ||
| 'setup.cfg': setup_config}) | ||
|
|
||
| with pytest.raises(AssertionError) as exception: |
There was a problem hiding this comment.
| with pytest.raises(AssertionError) as exception: | |
| with pytest.raises(ValueError): |
I'm a bit distured that this was succeeding in the first place, as this should be raising ValueError.
|
Hey! @pganssle I think this is a solution. Lemme know if you have suggestions/comments about it |
|
Hey @pganssle Just wondering if you had any comments on this PR? Or is it good to go? |
|
@RajdeepRao Gah! Sorry for the delay, this fell off my radar. I'll put it on my to-do list to review for today. |
The ability to handle files was originally added and documented based on a misunderstanding of what the `license` field should include. The field should be the name of the license, not the full text. It is likely that anyone actually using this was outputing malformed PKG-INFO files, because most license files contain newlines. See GH issue pypa#1551
Both the old and new approaches are deeply unsatisfying to me, but without reworking how these test commands are run, I think this is about as close as we can get to enforcing that this specific call raises ValueError.
|
Thanks for your diligence on this issue @RajdeepRao, the testing part in particular was a tough one. I have retooled the way the exception is tested in a way that is not entirely to my satisfaction, but it at least moves the special-casing of "ValueError" into the only test actually using that special case. I've also changed marked the change as "breaking" rather than deprecation. It's not a deprecation because we're actually just raising an error rather than saying that it will eventually be an error. We can either leave it as "breaking" or make it "change". The argument in favor of it being breaking is that this was a documented way to use Under @jaraco 's rule of "anything that would require human intervention for is a breaking change", this is breaking because people used to be able to generate otherwise valid packages with broken |
|
Sorry about the delayed response, didn't have my laptop on me. Yeahh the special casing of "ValueError" seemed a little more generic than it should've been. I also agree with you to err to the side of 'breaking' than deprecation if it changes the expected behavior from before this change. It's been a pleasure working on this and a great kickstarter open source project. I'm def looking forward to make more contributions, Thanks a ton |
Summary of changes
This pull request disallows the file inputs for
licensefield in thesetup.cfg. Devs would have to input a string.Closes #1551
Pull Request Checklist