Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
added --no-run option for rustdoc #83857
added --no-run option for rustdoc #83857
Changes from all commits
db6a916
0f3efe2
a6bf81b
72f534a
b08b484
08c7f97
3ad3597
4770446
202659a
3273d2f
03c710b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
If it didn't run why is it "ok"? IIRC that is the same as test passing right?
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.
so adding the
--no-run
flag change the behavior of the test like it was annotated as ```no_run. Maybe this is unwanted and something else should be displayed.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.
Ping @CraftSpider @jyn514 should we have a different text? "ok" may be confusing since it didn't run. Also note that we may also want a different color or the same color as "ignored".
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.
Hm. I don't think 'ignored' is quite right, but 'ok' is also a bit confusing. If we can make it something new, maybe something like 'built' or 'checked'? If not, I think 'ok' is more accurate than 'ignored'.
Color-wise, I'm fine with it being ok colored, as it is a requested change to behavior, not implicit, so it's requested to treat it as success. If that makes sense.
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.
After investigations I think that in order to do so, I have to modify the lib test. This will also affect all tests and not just doctests. Do you think this is a worthwhile investment and should this be in a different PR ? Anyway I would be interested in implementing that but the scope is a bit bigger than I anticipated.
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.
no_run
by itself is useless.To summarize:
rustdoc -Z unstable-options --no-run
it just build the documentation, it has the same behavior as justrustdoc
rustdoc -Z unstable-options --no-run --test
run the doctest set. It behave as if every test had the no_run meaning they won't run. Meaning that test like```
panic!()
```
will print as ok like
```no_run
panic!()
```
without the
--no-run
flagI agree that is confusing. Maybe a error should be raised if no_run is active and not
should_test
. However I think that if you runrustdoc -Z unstable-options --no-run
your intention was probably to run the test instead of documenting your project.I assume that in the original issue the final goal would be to be able to generate bin (with
--persist-doctests
) but without running them.Or maybe a another possible case would be to unify the behavior of
cargo test --no-run
(provided that down the line cargo is also modified). For instancecargo test --all
test both regular test and doctest butcargo test --all --no-run
skips doctestcargo test --doc --no-run
give an error.Now if the explicit goal is to harmonize with regular test the output should just be empty.
Another option would for me to change the test lib such that no_run test display something like
test foo ... checked
, provided that such change is desirable (to discuss thought the RFC process ?).Moreover test with
compile_fail
should also print something different maybe checked is also desirable.I am new contributing for rust so I am not quite sure of what to do to solve this issue.
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 think we should give a hard error here; if people meant to run the test they can add
--test
. Could you suggest adding--test
in the error message?See
rust/src/librustdoc/config.rs
Lines 587 to 593 in 202659a
I think all your other changes can go in a follow-up PR, I don't think we should be changing libtest here.
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.
Ok thank you. I have not change libtest yet I will work on that too :)
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 not change libtest in this pr. I am not a maintainer and don't know who to ask to approve; finding out will delay landing this.
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.
Sorry I was not clear. I will do another PR for libtest