Skip to content

str: Implement str::trim_newline #91047

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

Closed
wants to merge 3 commits into from

Conversation

ijackson
Copy link
Contributor

This function was contemplated here rust-lang/rfcs#3196 (comment)

It seems like a good idea to me. I chose to call it trim_newline rather than trim_end_newline since trimming newlines anywhere else would be weird, so end felt redundant.

I have not provided a method on String as contemplated in that issue. We don't have any other mutating versions of str trimming methods, so everyone who wants that has to write s.truncate(s.trim_wombat().len()). I think that's OK since it is more idiomatic to work with slices.

@rust-highfive
Copy link
Contributor

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 19, 2021
@ijackson
Copy link
Contributor Author

Obviously, if the basic idea seems good I will open the tracking issue and update the MR branch.

@ijackson ijackson changed the title str: Implement str::strip_newline str: Implement str::trim_newline Nov 19, 2021
@klensy
Copy link
Contributor

klensy commented Nov 19, 2021

This test will fail:

assert_eq!("Text", "Text\n\r".trim_newline());

Ahh,

    /// 'Newline' is precisely a newline character (`0xA`), perhaps
    /// preceded by a carriage return (`0xD`).  I.e., `'\r\n'` or
    /// `'\n'`.  (This is the same definition as used by [`str::lines`]

ok then.

@rust-log-analyzer

This comment has been minimized.

@ijackson
Copy link
Contributor Author

This test will fail:

assert_eq!("Text", "Text\n\r".trim_newline());

Ahh,

    /// 'Newline' is precisely a newline character (`0xA`), perhaps
    /// preceded by a carriage return (`0xD`).  I.e., `'\r\n'` or
    /// `'\n'`.  (This is the same definition as used by [`str::lines`]

Indeed. Your response seems to me to demonstrate that the docs needed improving :-). I have added the actual handling of this, with a note, to the example.

lines() treats "Text\n\r" as two lines, "Text" and "": https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=648cd55ce2178e7c3228e9f75d9b531b . Arguably the 2nd line should be "\r" since there was no LF but it is surely too late to change that now.

ijackson added a commit to ijackson/rust that referenced this pull request Nov 19, 2021
Apropos discussion here
  rust-lang#91047 (comment)

Sadly, str::lines gets this wrong.  I think it is probably too late to
fix this, so document it instead.
@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Nov 22, 2021
@dtolnay dtolnay added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 10, 2021
@JohnCSimon JohnCSimon 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 Jan 30, 2022
@JohnCSimon
Copy link
Member

Ping from triage:
@ijackson Can you post your status on this PR? thanks

@Dylan-DPC
Copy link
Member

Closing this as inactive

@Dylan-DPC Dylan-DPC closed this Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants