-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Improve doc of some methods that take ranges #140983
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
r? @ibraheemdev rustbot has assigned @ibraheemdev. Use |
@rustbot label +A-docs |
r? @hkBst |
Failed to set assignee to
|
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'm happy with this.
/// Panics if the range has `start_bound > end_bound`, or, a range bound is | ||
/// bounded and greater than the length of the deque. |
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.
", or, a range bound is bounded and greater than ..." doesn't read quite right to me. Suggestion: ", or if the range has an end bound that is greater than the length of the dequeu".
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.
@tgross35 the problem with that is that it's not as precise as the previous one for 2 reasons:
- If the end bound is unbounded, do we consider that greater than the length ? Unbounded kind of means "infinite" so you could understand from your suggestion that if it's unbounded it will panic which is false
- The method can also panic if it's the start bound that is greater than the length of the deque
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 the end bound is unbounded, do we consider that greater than the length
Users are more likely to think of things with regard to the range types, which may have "no end bound", not an "unbounded end bound" which this current PR's wording matches. I assume this comes from RangeBounds::end_bound
always returning a Bound { Included, Excluded, Unbounded }
; this type is better thought of as a flattened form of Option<enum { Included, Excluded }>
, however, it is possible for an impl RangeBounds
type to have "no end bound" (the docs for Unbounded
say "Indicates that there is no bound in this direction.", after all).
The method can also panic if it's the start bound that is greater than the length of the deque
", or if the range has an end a start or end bound that is greater than the length of the dequeu" then? I'm open to other suggestions, but the "range bound is bounded" bit should change. (Also s/, or,/, or if
)
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 can kind of agree with the first point, tho I'm not sure how you can put it into words so it's easier to understand.
And for the second point, well then:
, or if the range has a start or end bound that is greater than
v
, or if the range has a bound that is greater than
v
, or if a range bound is greater than
which is almost the same thing as the current documentation without the "is bounded" part (tho I agree on the sed part)
Some methods that were taking some range in parameter were a bit inconsistent / unclear in the panic documentation.
Here is the recap:
RangeBounds
naming (it's also easier to understand I think)String
methods weren't mentionning that the method panics ifstart_bound > end_bound
but it actually does! It usesslice::range
which panics whenstart > end
. (https://doc.rust-lang.org/stable/src/alloc/string.rs.html#1932-1934, https://doc.rust-lang.org/stable/src/core/slice/index.rs.html#835-837).You can also test it with: