-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
std: Account for CRLF in {str, BufRead}::lines #28034
Conversation
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
/// | ||
/// This does not include the empty string after a trailing `\n`. | ||
/// This does not include the empty string after a trailing newline or CRLF. |
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 should clarify (like the BufRead
one does) that the newlines are stripped from the strings that are yielded.
How should this change be publicized? Since it's silently breaking we need to spread the word to prevent surprises. |
Based on this comment I can't tell what you think of this immediate deprecation. It sounds to me that you agree we should not deprecate it now but want to anyway. |
I want to deprecate it now. I understand that it'll be annoying but I don't want to prevent this and all future deprecations until we have a system in place for delaying deprecation warnings. |
@bors: r+ |
📌 Commit 76011a2 has been approved by |
On the issue of deprecation timing: it's more relevant when we're introducing a new stable function. In this case, we're deprecating in favor of an existing function, and we should give people a heads-up about this new behavior ASAP. As far as announcing this goes, I think we should send a blast on all of our usual channels (reddit, discuss, twitter, IRC) once it lands, and then highlight it in the relnotes. I'm tagging accordingly. |
☔ The latest upstream changes (presumably #28200) made this pull request unmergeable. Please resolve the merge conflicts. |
This commit is an implementation of [RFC 1212][rfc] which tweaks the behavior of the `str::lines` and `BufRead::lines` iterators. Both iterators now account for `\r\n` sequences in addition to `\n`, allowing for less surprising behavior across platforms (especially in the `BufRead` case). Splitting *only* on the `\n` character can still be achieved with `split('\n')` in both cases. The `str::lines_any` function is also now deprecated as `str::lines` is a drop-in replacement for it. [rfc]: https://github.com/rust-lang/rfcs/blob/master/text/1212-line-endings.md Closes rust-lang#28032
76011a2
to
48615a6
Compare
This commit is an implementation of [RFC 1212][rfc] which tweaks the behavior of the `str::lines` and `BufRead::lines` iterators. Both iterators now account for `\r\n` sequences in addition to `\n`, allowing for less surprising behavior across platforms (especially in the `BufRead` case). Splitting *only* on the `\n` character can still be achieved with `split('\n')` in both cases. The `str::lines_any` function is also now deprecated as `str::lines` is a drop-in replacement for it. [rfc]: https://github.com/rust-lang/rfcs/blob/master/text/1212-line-endings.md Closes #28032
This commit is an implementation of RFC 1212 which tweaks the behavior of
the
str::lines
andBufRead::lines
iterators. Both iterators now account for\r\n
sequences in addition to\n
, allowing for less surprising behavioracross platforms (especially in the
BufRead
case). Splitting only on the\n
character can still be achieved withsplit('\n')
in both cases.The
str::lines_any
function is also now deprecated asstr::lines
is adrop-in replacement for it.
Closes #28032