Skip to content
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

Add "check" command #395

Merged
merged 8 commits into from
Sep 16, 2018
Merged

Add "check" command #395

merged 8 commits into from
Sep 16, 2018

Conversation

di
Copy link
Member

@di di commented Sep 14, 2018

The twine check command determines whether a given distributions long_description will be properly rendered on PyPI.

By checking the metadata after the distribution has been built, this provides a way to check packages that have build-time dependencies that python setup.py check won't install (see pypa/readme_renderer#118).

This can eventually be extended to check other metadata as well, once this logic is moved out of Warehouse.

This test was fine as-is, but the `future` module that is a
subdependency of `readme_renderer` provides a `builtins` that lacks the
`__import__` attribute, causing this test to fail when the dependency is
added.

Instead, simplify the check for the keyring module, and the test as
well.
@theacodes
Copy link
Member

This mostly LGTM, but I'm a little concerned about adding a dependency on readme_renderer since it pulls in a c dependency. Are we at all concerned about systems/environments where that won't work? Or do we feel comfortable with it since this is a developer tool?

@di
Copy link
Member Author

di commented Sep 14, 2018

The built distribution support for cmarkgfm is pretttttty good (thanks to you @theacodes!) so I'm not really worried about it.

Copy link
Member

@theacodes theacodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'd also like @sigmavirus24 to take a look.

@sigmavirus24
Copy link
Member

I would love for this to extend to other metadata as well. 👍 :shipit:

@theacodes theacodes merged commit cdd5b2d into pypa:master Sep 16, 2018
@theacodes
Copy link
Member

Thank you, @di!

@gaborbernat
Copy link

gaborbernat commented Sep 20, 2018

Can we release this? Thank you very much!

PS. Never mind saw #554.

@@ -72,4 +72,4 @@ def dispatch(argv):

main = registered_commands[args.command].load()

main(args.args)
return main(args.args)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very breaking change since the CLI now returns 1 (which is the standard code for "failure") on a successful execution.

di added a commit to di/twine that referenced this pull request Sep 24, 2018
This was added accidentally in pypa#395 and was missed on review.
@di di mentioned this pull request Sep 24, 2018
theacodes pushed a commit that referenced this pull request Sep 24, 2018
* Fix a bad test

The assertion on the exception message was never being run

* Add failing test for upload

In the event that the upload is successful, the return should be
something falsey (such as the implicit None) as this value will
eventually be used to determine the exit code.

* Remove accidental return statement from upload cmd

This was added accidentally in #395 and was missed on review.

* Update changelog
try:
import keyring
except ImportError:
if 'keyring' not in sys.modules:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How will 'keyring' be in sys.modules before it is imported?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An excellent point...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will create an issue 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need, I'm already fixing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants