-
Notifications
You must be signed in to change notification settings - Fork 13.4k
[rustdoc] Give more information into extracted doctest information #141399
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
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
cd34db0
to
4e95830
Compare
Fixed |
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.
Core logic looks correct (mostly just exposing existing variables), just have a few nits about naming to hopefully increase readability.
4e95830
to
9888eb4
Compare
Updated. |
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.
Looks pretty good, I've just got a few more notes about naming, most of which are extrapolations of previous suggested changes.
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.
This also needs changes to the docs for this feature, which are in the rustdoc book.
7447a70
to
d7877b0
Compare
This comment has been minimized.
This comment has been minimized.
d7877b0
to
38947d8
Compare
Applied suggestions! |
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 don't think we should be putting the test stdout directly into the docs, at least not unmodified
"edition": null, | ||
"added_css_classes": [], | ||
"unknown": [] | ||
"file":"$DIR/extract-doctests-result.rs", |
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 make it clear that the filename here is the filename of the file that contains the doctest. Since we're using the name src/lib.rs
above, shouldn't we use the same name here?
also, aren't the $DIR
placeholders are added by compiletest, not rustdoc?
"returns_result":true | ||
} | ||
}, | ||
"name":"$DIR/extract-doctests-result.rs - (line 8)" |
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 don't think copy-pasting the JSON from extract-doctests-results.stdout
is 100% correct here, since things like line numbers won't be the same.
ec02beb
to
d3bf6d5
Compare
I fixed the book example. |
d3bf6d5
to
e361171
Compare
e361171
to
d3bf6d5
Compare
d3bf6d5
to
0d048d5
Compare
cool, this seems really useful! |
For now I only know rust-for-linux as potential users, we'll see if more users will appear in the future. |
Follow-up of #134531.
This update fragment the doctest code into its sub-parts to give more control to the end users on how they want to use it.
The new JSON looks like this:
for this doctest:
With this, I think it matches what you need @ojeda? If so, once merged I'll update the patch I sent to RfL.
r? @aDotInTheVoid