Improve documentation for slice strip_* functions #75078

ijackson · 2020-08-03T00:33:36Z

Prompted by the stabilisation tracking issue #73413 I looked at the docs for strip_prefix and strip_suffix for both str and slice, and I felt they could be slightly improved.

Thanks for your attention.

rust-highfive · 2020-08-03T00:33:38Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

sfackler · 2020-08-03T00:54:33Z

r? @rust-lang/docs

steveklabnik

Seems fine, but I have a few questions :)

steveklabnik · 2020-08-03T20:54:28Z

library/core/src/str/mod.rs

-    /// If the string starts with the pattern `prefix`, `Some` is returned with the substring where
-    /// the prefix is removed. Unlike `trim_start_matches`, this method removes the prefix exactly
-    /// once.
+    /// If the string starts with the pattern `prefix`, returns


these lines are wrapped much, much shorter than the old ones were. What column did you wrap these to?

steveklabnik · 2020-08-03T20:54:50Z

library/core/src/slice/mod.rs

+    ///
+    /// If the slice does not start with `prefix`, returns `None`.
+    ///
+    /// (If `prefix` is empty, simply returns the original slice.)


No need for parenthesis around these.

ijackson · 2020-08-04T09:27:58Z

Steve Klabnik writes ("Re: [rust-lang/rust] Slice strip (#75078)"):

Seems fine, but I have a few questions :)

Thanks for the review.

these lines are wrapped much, much shorter than the old ones were. What column did you wrap these to?

Yes. 75. If I weren't effectively rewriting it I wouldn't have reformatted it. I ran it through rustfmt and it didn't change the line length. Did I miss a style rule ? IME line length is one of the major areas of style dispute so I expected rustfmt to wrap my comment if it disagreed with my preference of 75.

+ /// (If `prefix` is empty, simply returns the original slice.) No need for parenthesis around these.

Without the parens, it reads as if this is a third case, "If X... . If Y ... . If Z ...". If read as a third case, it's confusing because it makes the cases not mutually exclusive, and because it omits the fully formal language mentioning `Some`. As a parenthetical comment it's much less susceptible to that misreading. The parens also hint that maybe the reader can skip this part. I considered removing it entirely but that felt too austere. Perhaps there is some other way to indicate its status. Prefixing it with "Therefore;" or "Note:" or something maybe. Thanks, Ian.

…

-- Ian Jackson <ijackson@chiark.greenend.org.uk> These opinions are my own. Pronouns: they/he. If I emailed you from @fyvzl.net or @evade.org.uk, that is a private address which bypasses my fierce spamfilter.

LeSeulArtichaut · 2020-08-22T17:34:56Z

Ping from triage: @steveklabnik could you provide feedback/guidance about the last comment? Thanks in advance!

tesuji · 2020-09-13T14:07:53Z

r? @jyn514 for reviewing documentation improvement for unstable slice methods: strip_*.

jyn514 · 2020-09-13T16:08:26Z

IME line length is one of the major areas of
style dispute so I expected rustfmt to wrap my comment if it disagreed
with my preference of 75.

I think rustfmt will wrap comments if they're longer, but not if they're shorter. So you should match the existing style and only trim it down as necessary (I think the standard library uses 100 columns?)

No need for parenthesis around these.
Without the parens, it reads as if this is a third case

Maybe make it a doc-test instead of a parenthetical? Then it will be tested, too.

steveklabnik · 2020-09-14T13:35:49Z

Yes. 75.

If I weren't effectively rewriting it I wouldn't have reformatted it. I ran it through rustfmt and it didn't change the line length. Did I miss a style rule ? IME line length is one of the major areas of style dispute so I expected rustfmt to wrap my comment if it disagreed with my preference of 75.

So here's my recollection: the standard rustfmt style is 100, but the standard library uses 80. However, the rustfmt.toml doesn't include a setting here; that may be something they overlooked. Additionally, I think rustfmt only reformats things that are too long, not too short. Which is maybe not ideal, but I don't think it expects people to manually wrap shorter.

@rust-lang/libs should really answer this, I think.

Without the parens, it reads as if this is a third case,

It pretty much is, though. Regardless, it being both its own paragraph and in ()s feels very strange.

ijackson · 2020-09-14T15:55:42Z

Steve Klabnik writes ("Re: [rust-lang/rust] Slice strip (#75078)"):

So here's my recollection: the standard rustfmt style is 100, but the standard library uses 80. However, the rustfmt.toml doesn't include a setting here; that may be something they overlooked. Additionally, I think rustfmt only reformats things that are too long, not too short. Which is maybe not ideal, but I don't think it expects people to manually wrap shorter.

I don't mind rewrapping to 100, if that's the house style. Unless I hear otherwise I will do that.

Without the parens, it reads as if this is a third case, It pretty much is, though. Regardless, it being both its own paragraph and in ()s feels very strange.

I'm afraid I must definitely disagree. It is not a third case: it is a subset of the first case. The empty slice can be found at the start of any slice and removing it leaves the original input. Treating this as a separate case is confusing because it suggests to the reader that they must have misunderstood the first case. I will see if I can find a way to different way to write it that may suit you better but also avoid what I see as confusing or awkward phrases. Thanks, Ian.

…

-- Ian Jackson <ijackson@chiark.greenend.org.uk> These opinions are my own. Pronouns: they/he. If I emailed you from @fyvzl.net or @evade.org.uk, that is a private address which bypasses my fierce spamfilter.

LukasKalbertodt · 2020-09-20T09:59:31Z

I don't mind rewrapping to 100, if that's the house style.

Yes please 😋 The doc comments wrapping in this file is a bit inconsistent unfortunately. Some are wrapped at column 80, others at 100. So either are fine, I guess.

Regarding the "number of cases" discussion: I would simply put all of the sentences in one paragraph and remove the parenthesis. Three sentences is certainly not too long for a paragraph.

crlf0710 · 2020-10-08T12:45:04Z

@ijackson Ping from triage: any updates on this?

ijackson · 2020-10-08T14:05:36Z

Charles Lew writes ("Re: [rust-lang/rust] Improve documentation for slice strip_* functions (#75078)"):

@ijackson Ping from triage: any updates on this?

Hi. Sorry, yes, this is still on my radar but I haven't got round to it. Possibly at the weekend.

…

-- Ian Jackson <ijackson@chiark.greenend.org.uk> These opinions are my own. Pronouns: they/he. If I emailed you from @fyvzl.net or @evade.org.uk, that is a private address which bypasses my fierce spamfilter.

"Some is returned with <some value>" is an awkward construction. The use of the passive voice is a bit odd, and doesn't seem like the house style. So say instead "returns X, wrapped in `Some`", for which there is some other precedent in stdlib. Instead of repeating "with the prefix removed", say "after the prefix". This is a bit clearer that the original is not modified. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

The stabilisation issue, rust-lang#73413, has an open item for documentation. I looked at the docs and it is all there, but I felt it could do with some minor wording improvement. I looked at the `str::strip_prefix` docs for a template. (That resulted in me slightly changing that doc too.) I de-linkified `None` and `Some`, as I felt that rather noisy.. I searched stdlib, and these don't seem to be usually linkified. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

@LukasKalbertodt

Requested-by: @LukasKalbertodt Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

@LukasKalbertodt

Requested-by: @LukasKalbertodt Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

@LukasKalbertodt

Roughly as requested by @LukasKalbertodt. I still prefer clearly making these two cases. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

ijackson · 2020-10-12T15:50:45Z

I have rewrapped and reorganised this now. I hope you like it better. Sorry for the delay...

ijackson · 2020-10-28T18:12:56Z

@steveklabnik Hi, did you get a chance to look at my revised version? Thanks.

jyn514 · 2020-10-28T20:58:00Z

@ijackson FYI you can change the labels yourself with @rustbot modify labels: +S-waiting-on-review -S-waiting-on-author. Then it shows up in the review queue and the reviewer doesn't have to look at each notification to know if the PR is waiting on them.

steveklabnik · 2020-10-29T15:19:19Z

Thank you!

@bors: r+ rollup

bors · 2020-10-29T15:19:21Z

📌 Commit 22358c6 has been approved by steveklabnik

…as-schievink Rollup of 11 pull requests Successful merges: - rust-lang#75078 (Improve documentation for slice strip_* functions) - rust-lang#76138 (Explain fully qualified syntax for `Rc` and `Arc`) - rust-lang#78244 (Dogfood {exclusive,half-open} ranges in compiler (nfc)) - rust-lang#78422 (Do not ICE on invalid input) - rust-lang#78423 (rustc_span: improve bounds checks in byte_pos_to_line_and_col) - rust-lang#78431 (Prefer new associated numeric consts in float error messages) - rust-lang#78462 (Use unwrapDIPtr because the Scope may be null.) - rust-lang#78493 (Update cargo) - rust-lang#78499 (Prevent String::retain from creating non-utf8 strings when abusing panic) - rust-lang#78505 (Update Clippy - temporary_cstring_as_ptr deprecation) - rust-lang#78527 (Fix some more typos) Failed merges: r? `@ghost`

Stabilize slice::strip_prefix and slice::strip_suffix These two methods are useful. The corresponding methods on `str` are already stable. I believe that stablising these now would not get in the way of, in the future, extending these to take a richer pattern API a la `str`'s patterns. Tracking PR: rust-lang#73413. I also have an outstanding PR to improve the docs for these two functions and the corresponding ones on `str`: rust-lang#75078 I have tried to follow the [instructions in the dev guide](https://rustc-dev-guide.rust-lang.org/stabilization_guide.html#stabilization-pr). The part to do with `compiler/rustc_feature` did not seem applicable. I assume that's because these are just library features, so there is no corresponding machinery in rustc.

rust-highfive assigned sfackler Aug 3, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 3, 2020

ijackson mentioned this pull request Aug 3, 2020

Tracking Issue for slice_strip feature #73413

Closed

3 tasks

rust-highfive unassigned sfackler Aug 3, 2020

steveklabnik reviewed Aug 3, 2020

View reviewed changes

JohnTitor assigned steveklabnik Aug 6, 2020

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 12, 2020

jyn514 added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Sep 13, 2020

jyn514 changed the title ~~Slice strip~~ Improve documentation for slice strip_* functions Sep 14, 2020

crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 8, 2020

ijackson added 5 commits October 12, 2020 14:47

docs: Rewrap str::strip_prefix and strip_suffix back to 100

4549c77

Requested-by: @LukasKalbertodt Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

docs: Rewrap slice::strip_prefix and strip_suffix back to 100

6f5e96f

Requested-by: @LukasKalbertodt Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

docs: slice::strip_prefix and strip_suffix, fold in sentence

22358c6

Roughly as requested by @LukasKalbertodt. I still prefer clearly making these two cases. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

ijackson force-pushed the slice-strip branch from 4370322 to 22358c6 Compare October 12, 2020 13:49

ijackson mentioned this pull request Oct 12, 2020

Stabilize slice::strip_prefix and slice::strip_suffix #77853

Merged

jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 28, 2020

steveklabnik approved these changes Oct 29, 2020

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 29, 2020

jonas-schievink mentioned this pull request Oct 29, 2020

Rollup of 11 pull requests #78528

Merged

bors merged commit 2168210 into rust-lang:master Oct 30, 2020

rustbot added this to the 1.49.0 milestone Oct 30, 2020

ijackson deleted the slice-strip branch December 1, 2020 11:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve documentation for slice strip_* functions #75078

Improve documentation for slice strip_* functions #75078

ijackson commented Aug 3, 2020 •

edited

Loading

rust-highfive commented Aug 3, 2020

sfackler commented Aug 3, 2020

steveklabnik left a comment

steveklabnik Aug 3, 2020

steveklabnik Aug 3, 2020

ijackson commented Aug 4, 2020 via email

LeSeulArtichaut commented Aug 22, 2020

tesuji commented Sep 13, 2020

jyn514 commented Sep 13, 2020 •

edited

Loading

steveklabnik commented Sep 14, 2020

ijackson commented Sep 14, 2020 via email

LukasKalbertodt commented Sep 20, 2020

crlf0710 commented Oct 8, 2020

ijackson commented Oct 8, 2020 via email

ijackson commented Oct 12, 2020

ijackson commented Oct 28, 2020

jyn514 commented Oct 28, 2020

steveklabnik commented Oct 29, 2020

bors commented Oct 29, 2020

Improve documentation for slice strip_* functions #75078

Improve documentation for slice strip_* functions #75078

Conversation

ijackson commented Aug 3, 2020 • edited Loading

rust-highfive commented Aug 3, 2020

sfackler commented Aug 3, 2020

steveklabnik left a comment

Choose a reason for hiding this comment

steveklabnik Aug 3, 2020

Choose a reason for hiding this comment

steveklabnik Aug 3, 2020

Choose a reason for hiding this comment

ijackson commented Aug 4, 2020 via email

LeSeulArtichaut commented Aug 22, 2020

tesuji commented Sep 13, 2020

jyn514 commented Sep 13, 2020 • edited Loading

steveklabnik commented Sep 14, 2020

ijackson commented Sep 14, 2020 via email

LukasKalbertodt commented Sep 20, 2020

crlf0710 commented Oct 8, 2020

ijackson commented Oct 8, 2020 via email

ijackson commented Oct 12, 2020

ijackson commented Oct 28, 2020

jyn514 commented Oct 28, 2020

steveklabnik commented Oct 29, 2020

bors commented Oct 29, 2020

ijackson commented Aug 3, 2020 •

edited

Loading

jyn514 commented Sep 13, 2020 •

edited

Loading