-
Notifications
You must be signed in to change notification settings - Fork 308
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
Don't crash if there is no description #412
Conversation
Codecov Report
@@ Coverage Diff @@
## master #412 +/- ##
=========================================
+ Coverage 78.08% 78.29% +0.2%
=========================================
Files 14 14
Lines 730 737 +7
Branches 104 106 +2
=========================================
+ Hits 570 577 +7
- Misses 127 128 +1
+ Partials 33 32 -1
Continue to review full report at Codecov.
|
twine/commands/check.py
Outdated
@@ -94,7 +94,7 @@ def check(dists, output_stream=sys.stdout): | |||
|
|||
# Actually render the given value | |||
rendered = renderer.render( | |||
metadata.get("description"), stream=stream, **parameters | |||
metadata.get("description") or "", stream=stream, **parameters |
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.
It would be better if this was done outside of the function call.
It would further seem as though "description"
may not be present and if it is present (or perhaps it is always present?) it can be None
. The behaviour of metadata
seems to be poorly understood by all involved, what do you think about documenting the actual behaviour here. It would be better if everyone after this point in time understood the reason for this change and why it was necessary. The comment in the test is a bit too opaque to provide that understanding.
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.
I was following the precedent from line 89, would you like me to change that one as well?
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.
Please do. It's not a very good precedent.
Since all this command does right now is check if the description is valid, perhaps it should give a warning if there is no |
I agree with the warning or error. For example, I found this bug and PR because twine does not include long_description in generated sdists (pypa/flit#216); if twine had seemed to work I would not have seen the flit problem. |
This is pretty special / broken from the setuptools / wheel side. Here's some data:
Things to note here:
|
Here's the new behaviour: $ twine check $(find */dist/* -type f | sort)
Checking distribution long/dist/pkg-0.0.0-py3-none-any.whl: Passed
Checking distribution missing/dist/pkg-0.0.0-py3-none-any.whl: warning: `long_description_content_type` missing. defaulting to `text/x-rst`.
warning: `long_description` missing.
Passed
Checking distribution missing_long/dist/pkg-0.0.0-py3-none-any.whl: warning: `long_description` missing.
Passed
Checking distribution missing_type/dist/pkg-0.0.0-py3-none-any.whl: warning: `long_description_content_type` missing. defaulting to `text/x-rst`.
Passed
Checking distribution long/dist/pkg-0.0.0.tar.gz: Passed
Checking distribution missing/dist/pkg-0.0.0.tar.gz: warning: `long_description_content_type` missing. defaulting to `text/x-rst`.
warning: `long_description` missing.
Passed
Checking distribution missing_long/dist/pkg-0.0.0.tar.gz: warning: `long_description` missing.
Passed
Checking distribution missing_type/dist/pkg-0.0.0.tar.gz: warning: `long_description_content_type` missing. defaulting to `text/x-rst`.
Passed |
@sigmavirus24 want to take another pass at this? |
Resolves #411