Improve the check command output

[pradyunsg]

Old:

New:

I think that's an improvement. :)

---

44ff389 prints a message, when there are no distributions given to check.

136de41 fixes a bug, where the errors in a distribution's markup, would flow through to the next distribution's errors. This was because they used the same stream.

Let me know if anyone prefers that I break these two out into separate PRs.

---

I can add colours to "FAILED", "PASSED" and "PASSED, with warnings" in a follow up PR, y'all are cool with it. I didn't in this PR, since I don't know if the maintainers are OK with another dependency. :)

---

I suggest reviewing this commit-by-commit. :)

[codecov]

Codecov Report

Merging #488 into master will increase coverage by 0.29%.
The diff coverage is 97.56%.

@@            Coverage Diff             @@
##           master     #488      +/-   ##
==========================================
+ Coverage   84.62%   84.91%   +0.29%     
==========================================
  Files          14       14              
  Lines         787      809      +22     
  Branches      115      121       +6     
==========================================
+ Hits          666      687      +21     
  Misses         85       85              
- Partials       36       37       +1

Impacted Files	Coverage Δ
twine/commands/check.py	98.64% <97.56%> (-1.36%)	⬇️

---

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 182e8af...06e8380. Read the comment docs.

[codecov]

Codecov Report

Merging #488 into master will increase coverage by 0.29%.
The diff coverage is 97.56%.

@@            Coverage Diff             @@
##           master     #488      +/-   ##
==========================================
+ Coverage   84.62%   84.91%   +0.29%     
==========================================
  Files          14       14              
  Lines         787      809      +22     
  Branches      115      121       +6     
==========================================
+ Hits          666      687      +21     
  Misses         85       85              
- Partials       36       37       +1

Impacted Files	Coverage Δ
twine/commands/check.py	98.64% <97.56%> (-1.36%)	⬇️

---

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 182e8af...a07d35f. Read the comment docs.

[pradyunsg]

Omigod. CI that gives a result in 1 minute, not 1 hour. :o

I need this more often in my life.

[sigmavirus24]

See the in-line comments. Also, I wonder if there are docs that need to be updated to explain the nuance between PASSED and PASSED, with warnings. Since not everyone using twine speaks English (or understands it as a first language)

[sigmavirus24]

I'm also a bit worried - given that the textual output is really our primary API for most people - that changing the formatting is going to break things.

[sigmavirus24]

I can add colours to "FAILED", "PASSED" and "PASSED, with warnings" in a follow up PR, y'all are cool with it

That would require automatic detection to disable colours when the output stream is not a TTY. Alternatively, I think we'd need to add configurable options for disabling that. And as soon as we start giving people this kind of configuration, I worry that people will want to eventually work on their own colourschemes or something equally far-fetched. I'll leave decisions about that to @di and @jaraco

[pradyunsg]

I'm also a bit worried - given that the textual output is really our primary API for most people - that changing the formatting is going to break things.

Uh... the current output doesn't have much (any?) structure to it? "warning: " shows up in the same line as the Passed/Failed are supposed to be (based on what happens when there are no warnings), which pushes Passed or Failed to a new line. The same stream is used for rendering multiple READMEs, resulting in duplicated errors in later distributions -- assuming they have the same READMEs.

Further, the output isn't documented anywhere as "will not change".

I wonder if there are docs that need to be updated to explain the nuance between PASSED and PASSED, with warnings

Well, there aren't too many docs for twine check in the first place. It's only mentioned once in the README, and that too without output. :)

I'm not super stoked about requiring new documentation as a part of this PR -- we can tackle improving twine check documentation in a follow up.

reg: colours in CLI

I know I brought it up -- I also don't want that to mix-up in the discussion of output changes.

Let's discuss this elsewhere, perhaps in a new issue?

[sigmavirus24]

Uh... the current output doesn't have much (any?) structure to it?

But it's consistent in its (crappy) behaviour, right? Thus there's implicit structure

Further, the output isn't documented anywhere as "will not change".

Regardless of whether it's documented as "will not change" people build tooling and automation around it. It's not an unfair assumption.

I'm not super stoked about requiring new documentation as a part of this PR

Documentation is critical to the maintainability of the project. Given there isn't much documentation that exists, is it really that opprobrious to ask for documentation as part of this pull request?

Let's discuss this elsewhere, perhaps in a new issue?

Yeah, I'm happy to discuss things in other places.

[pradyunsg]

But it's consistent in its (crappy) behaviour, right? Thus there's implicit structure

Okay, I'm confused.

Are you suggesting that twine should maintain it's current "crappy" behavior because someone might be parsing this output?

If so, that's fine. I can live with with that.

I felt like this change is an improvement for literally everyone else, and for folks are parsing this output, they'd likely appreciate intentional structure in the output; even if it breaks things for them right now.

Documentation is critical to the maintainability of the project.

I strongly agree. I've tackled things like pypa/pip#5526 precisely because they make life easier for maintainers, like us. :)

Given there isn't much documentation that exists, is it really that opprobrious to ask for documentation as part of this pull request?

I wouldn't call it opprobrious but, yes, it's unfair to hold this PR to a higher standard than status quo, for a thing it's not directly intended to improve/affect.

Additionally, I prefer PRs be scoped to do single tasks, so that an independently improvable thing doesn't block them. To be abundantly clear, I'm not opposed to improving twine check's documentation. I just don't want that to block this change, which I do view as an independent improvement.

---

I'm sorry, but the discussion is getting meta and I genuinely don't have the mental bandwidth to try to push through a meta discussion for this.

I'm happy to let this PR sit open, until y'all figure stuff out -- lemme know if there's any code changes/output formatting changes desired here. Or... if y'all decide that the current output is good enough.

[pradyunsg]

Poking this again to see if any @pypa/twine-maintainers have inputs here.

I'd really like to see this get improved since the current output isn't very nice.

[bhrutledge]

@pradyunsg Thanks for the nudge, and your patience. I like the new output, but think it still needs buy-in from other maintainers. To potentially mitigate the backwards-compatibility concern, I've proposed releasing it as part of Twine 2.0.

[pradyunsg]

Thanks @bhrutledge!

I strongly warn against viewing this as a backward incompatible change, since it sets a precedence for not being able to change twine's output at all, even for significant improvements to "crappy" output (or for bug fixes? idk where @sigmavirus24 is drawing the line). IMO that's not a good spot to be in but that's something for the maintainers to decide upon.

Anyway, since I won't be dealing with the consequences of that decision, I'm on board for that if viewing this as a backward incompatible change means that we roll it out quicker. Especially, if it's before I get frustrated enough to drop this, given how my experience contributing here has been.

---

To reiterate my thoughts on this, literally all users of twine check would benefit from this change, in the short term or, if they're doing something that's not explicitly mentioned as OK to do, in the medium term.

[di]

I'm in favor of this and don't see it as a breaking change. I've never seen anyone relying on the output from twine check and even if they are, I don't think that we should imply that this is a valid API here (aside from the exit code).

[bhrutledge]

Thanks again for the improvement. I added two small, non-blocking style questions.

I believe the concerns were addressed.

[pradyunsg]

Self-reviewed, so that I can commit suggested changes while using the GitHub's UI.