Rename "--display-warnings" to "--display-doctest-warnings" #73314
Conversation
rust-highfive
added
the
S-waiting-on-review
|
I'm unclear exactly how much of the issue remains unresolved. |
|
Me neither actually. Like I said, I don't consider remaining "issues" as issues. |
|
AFAICT that all makes sense. Do we need Cargo on-board with this or is it entirely independent? |
|
Let's ping them to be sure. :) cc @rust-lang/cargo |
|
Seems fine to me, I don't think it will affect cargo at all. I noticed that it doesn't seem to have an effect for a normal doc build (without |
|
@GuillaumeGomez Does the user have to invoke rustdoc manually in order to trigger this warning display mechanism? |
|
Actually I wonder if the option name is the best... Cargo will have to be updated to take it into account though. The equivalent would be cc @rust-lang/rustdoc @ehuss |
|
From what I can tell, this is different from I still don't see how this works for normal documentation. The docs added in this PR imply that I'm also not sure in what way cargo would need to know about this. Generally we don't add flags to |
|
Then is there anything else to be done here or can we move forward? |
ollie27
added
the
T-rustdoc
|
Looks good to me :) |
|
ping @Manishearth @ollie27 |
|
Could someone elaborate on what issues remain unresolved (whether we call them issues or not)? I so far have not been able to find any comment explaining that, and I feel mildly uncomfortable stabilizing something that multiple people agree has some problem without actually saying what that problem is. I also don't see any tests for this in-tree, and I feel like that would be good to add before stabilization to make sure it keeps working as we intend. It also looks like the option currently does not color the warnings, but I suppose that's #74293, right? |
|
I don't know if it's an issue considering that I treat separately. The purpose is different: one provides information and the other makes it look more beautiful. But we can wait on #74293 if people want? |
|
Can we call this |
|
@Manishearth It sounds much better, indeed! |
|
To reiterate the concerns from #41574 (comment) and #41574 (comment).
So overall, what actually are the use cases for |
|
Can we document everything you just said? I didn't know |
|
Looking at this a bit closer,
|
|
r? @jyn514 |
|
Going over ollie's comments again:
Hmm .. this is definitely true for when rustdoc checks the items (especially after #73566), but is it true for doctests? One of the main reasons I wanted this feature was to see warnings in doctests. If warnings are already on by default in doctests that sounds fine to me. I don't think we need a dedicated flag for hiding warnings while checking items.
This seems really hard to discover. There's a brief mention of Maybe this is a documentation issue? But I'd be very unhappy with |
GuillaumeGomez
force-pushed
the
display-warnings
branch
from
7dbb8d9
to
7590adc
Compare
|
Renamed the option and added a test (realized we didn't have one yet). |
Sorry, I wasn't clear: I was referring to the Which is a behavior change from what --display-warnings does currently I think. The goal is for |
|
It's the case already, no? Or did I miss something? |
|
Recapping one last time because it seems I misunderstood the first time:
I think having those both in one option is helpful. @rfcbot fcp merge |
|
@bors r+ |
GuillaumeGomez
force-pushed
the
display-warnings
branch
from
872d82f
to
b531a7f
Compare
|
Fixed conflict. @bors: r=jyn514 |
|
📌 Commit b531a7f has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit b531a7f with merge 18c85fa589bde20d19f5a94f97af428ef783cf86... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors: retry |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (c3c0f80): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
…eGomez Remove `--display-doctest-warnings` `--display-doctest-warnings` can be replicated in full with other existing features, there's no need to have a separate option for it. This removes the option and documents the combination of other features to replicate it. This also fixes a bug where `--test-args=--show-output` had no effect. cc `@ollie27,` rust-lang#73314 (comment) Fixes rust-lang#41574 r? `@GuillaumeGomez`
Fixes #41574.
cc @ollie27
r? @kinnison