-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Show the actual value of constant values in the documentation #66221
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
fb581b9
to
deb4f7a
Compare
I'm not sure we want this... Why not keeping the original value and putting it into a link to its definition instead? |
The motivating example here is I saw the tweet and it also happened to me multiple times so I figured I might as well fix it 🙂 Prior art: |
☔ The latest upstream changes (presumably #66233) made this pull request unmergeable. Please resolve the merge conflicts. |
216cfc6
to
b14632f
Compare
So in #53409 the problem seems to be over-detail when displaying a module, and this view it's not changed by this PR at all. For #42759, it's at least somewhat of an improvement because for non-scalar values we will mirror the source. However clearly the author of the issue wanted to see a value for the constant, so not showing anything is kind of a funny fix. #32735 is an ugly corner case, but I'll argue that there are more "should show the value of" than "should hide the value of" |
I still have issue with this PR: in case it is a mathematical value like PI, why would I want the full number and not the constant PI? const stuff: f64 = math::PI;
const stuff: f64 = 3.1415; I strongly prefer the first case. Even more considering that if you really want to see the full number, you can just click on the value... |
What if we showed it like this (approximated html):
Where the value part is only added for scalars? While remaining authentic to the original source code, it'll also solve the problem for |
Providing the "complete" value as a comment would be fine for me. For example:
|
ed5ef16
to
7b9ef3f
Compare
@GuillaumeGomez what do you think about this? It's a little noisy for some values (like |
7b9ef3f
to
d2d4429
Compare
In the first case, since this is already a value, there is no need to add the comment. Can you not provide it in case this is "direct" value? (no idea how to express correctly what I have in mind, I hope people will understand XD) |
6a89be2
to
5062d47
Compare
I changed the code to not show the computed value for consts which are literals (or I implemented a version (diff) which adds the |
5062d47
to
de2ff01
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
de2ff01
to
4200924
Compare
Instead of using HIR to check, can't you compare both displays and if they're the same, not print the comment one? |
This is what the current code does, but it doesn't catch everything (like |
Well I guess you can limit the display to nth first characters or something along the line? But yes, I think we're close to the end. Thanks so much for working on this! |
@GuillaumeGomez I added splitting for big numbers! Let me know what you think. |
@ohadravid Do you have screenshots by any chance? :) |
☔ The latest upstream changes (presumably #66984) made this pull request unmergeable. Please resolve the merge conflicts. |
8f2fd28
to
7a10e20
Compare
☔ The latest upstream changes (presumably #67202) made this pull request unmergeable. Please resolve the merge conflicts. |
Nice! Well then, unless @ollie27 wants something else to be updated, you can r=me once you have rebased. |
7a10e20
to
ea7b622
Compare
☔ The latest upstream changes (presumably #67540) made this pull request unmergeable. Please resolve the merge conflicts. |
ea7b622
to
53ff21e
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
53ff21e
to
e398f59
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
e398f59
to
531622a
Compare
531622a
to
811bdee
Compare
ping @ollie27? 🙏 |
@bors r+ rollup |
📌 Commit 811bdee has been approved by |
🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened |
Show the actual value of constant values in the documentation Fixes #66099, making rustdoc show evaluated constant scalar values. ![image](https://user-images.githubusercontent.com/2358365/68474827-c7a95e80-0226-11ea-818a-ded7bbff861f.png) where `main.rs` is ``` pub const VAL3: i32 = i32::max_value(); pub const VAL4: i32 = i32::max_value() - 1; ``` As a fallback, when a constant value is not evaluated (either because of an error or because it isn't a scalar), the original expression is used for consistency. I mimicked the way min/max values of integers are [`pretty_print`ed](https://github.com/rust-lang/rust/blob/master/src/librustc/ty/print/pretty.rs#L900), to show both the value a the "hint". While a little goofy for `std`, in user crates I think it's actually rather helpful.
☀️ Test successful - checks-azure |
Fixes #66099, making rustdoc show evaluated constant scalar values.
where
main.rs
isAs a fallback, when a constant value is not evaluated (either because of an error or because it isn't a scalar), the original expression is used for consistency.
I mimicked the way min/max values of integers are
pretty_print
ed, to show both the value a the "hint". While a little goofy forstd
, in user crates I think it's actually rather helpful.