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 option to check before uploading #415

Closed
wants to merge 1 commit into from
Closed

Add option to check before uploading #415

wants to merge 1 commit into from

Conversation

chadwhawkins
Copy link

Add an optional flag to the upload command to check the files before proceeding with the upload.

@codecov
Copy link

codecov bot commented Oct 24, 2018

Codecov Report

Merging #415 into master will decrease coverage by 0.36%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #415      +/-   ##
==========================================
- Coverage   78.29%   77.92%   -0.37%     
==========================================
  Files          14       14              
  Lines         737      743       +6     
  Branches      106      108       +2     
==========================================
+ Hits          577      579       +2     
- Misses        127      131       +4     
  Partials       33       33
Impacted Files Coverage Δ
twine/commands/upload.py 70.37% <33.33%> (-4.63%) ⬇️
twine/wininst.py 31.57% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd1d8b0...e0d1422. Read the comment docs.

Copy link
Member

@sigmavirus24 sigmavirus24 left a comment

Choose a reason for hiding this comment

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

I don't like the idea of having more than one way to do it.

@webknjaz
Copy link
Member

webknjaz commented Dec 7, 2018

@sigmavirus24 I think it's fine, it's meant to guard the upload process while check command could be used in the other gating layer separately (linting for example). Zen Of Python is not always the answer, especially if just used to attack practices. Objectively, there's a clear use case for such option and I support it therefore.
I'd say it's more of a plumbing vs. porcelain command layer distinction, which works well for another CLI tool called Git.

@jaraco
Copy link
Member

jaraco commented Sep 22, 2019

I agree with Ian here - composition is preferable to parameterization, and composition is what already exists. So right now, the current recommended usage is:

$ twine check ...
$ twine upload ...

The proposal is to add the ability to invoke that in one command:

$ twine upload --check-first ...

There are some nice things about that proposal: one command invocation, avoidance of repeating options.

To be honest, I'm a little surprised twine offers a check option and doesn't perform it before doing an upload (unconditionally). I personally have never run the check command, so I'm unsure what value it provides.

reads docs

Aha, so check performs a basic analysis that the docs will render properly. I've never had to run that because I use pytest-checkdocs.

It's arguable that twine shouldn't supply this functionality at all (that a separate command/process should be responsible for it). Given that twine does expose this functionality, it probably should do so in a way that's easily integrated into a deployment flow. Still, I don't think --check-first option to upload is the right model (consider instead a --then-upload option to the check command, which is the symmetric option).

Perhaps twine should consider allowing multiple commands to be invoked... something like twine check upload .... That's what setuptools did... but I'm not sure following setuptools' choices is necessarily a good approach.

it's meant to guard the upload process

That's the closest thing we have to describing the motivating use-case for this change... and that use-case seems easily satisfied by relying on the shell or orchestrating logic to fail on a failed twine check invocation.

Thanks for the proposal, but we will decline. If you feel there's a strong case for a single-command usage, make the case in an issue and we can discuss possible solutions.

@jaraco jaraco closed this Sep 22, 2019
@di
Copy link
Member

di commented Sep 23, 2019

Just one note here:

Aha, so check performs a basic analysis that the docs will render properly. I've never had to run that because I use pytest-checkdocs.

The idea is that twine check will eventually check all metadata, not just docs, see #430. Also, one key reason why this was moved from readme_renderer to twine is that twine check runs on actual distribution artifacts (like twine upload) in order to avoid needing to handle PEP 517/518 build-time dependencies.

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.

5 participants