Skip to content
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

Add a method `lines_any` to `BufRead` #26743

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
8 participants
@RalfJung
Copy link
Member

RalfJung commented Jul 2, 2015

This mirrors the presence of lines and lines_any on String. This fixes rust-lang/rfcs#1188.

Open questions:

  • Can I mark this function as stable immediately?
  • If possible, I'd rather patch lines to have the expected behavior of supporting both kinds of line endings properly. Unfortunately, with String, there already is a precedent of lines supporting only \n and lines_any additionally supporting \r\n. Is there any chance of changing the existing behavior?

Instead of writing a new iterator LinesAny, I could also use Lines and a map. It is unclear to me whether that would be any shorter. Let me know if you'd rather have it the other way.

Add a method `lines_any` to `BufRead`
This mirrors the presence of `lines` and `lines_any` on `String`
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jul 2, 2015

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @huonw (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. 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.

/// The iterator returned from this function will yield instances of
/// `io::Result<String>`. Each string returned will *not* have the newline
/// separator at the end.
#[unstable(feature = "io", reason = "Just recently added.")]

This comment has been minimized.

@alexcrichton

alexcrichton Jul 5, 2015

Member

We avoid blanket feature names nowadays, so can you rename this feature to lines_any?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 5, 2015

API and implementation-wise this looks fine to me, so tagging with T-libs and needs-decision.

@RalfJung

This comment has been minimized.

Copy link
Member Author

RalfJung commented Jul 6, 2015

I renamed the feature as requested.

Did you consider the proposal brought up by me here (and by others in #26710) to rather change the behavior of lines to what's arguably usually expected, namely, supporting both kinds of line-endings?

/// An iterator over the lines of an instance of `BufRead` split on either `\n` or `\r\n`.
///
/// See `BufRead::lines_any` for more information.
#[unstable(feature = "io", reason = "Just recently added.")]

This comment has been minimized.

@alexcrichton

alexcrichton Jul 6, 2015

Member

This and the tag below should also change to lines_any from io

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 6, 2015

I personally prefer this course of action as it mirrors what we do for strings already.

@RalfJung

This comment has been minimized.

Copy link
Member Author

RalfJung commented Jul 6, 2015

oops sorry for that, I pushed another one. Please tell me if I should smash those commits together; for now I sticked to the guide which said that I should always only add new ones.

Well, I'd argue that what we do for str does not match people's expectations. Of course the two should be consistent (which is why I wrote this pull request the way I did), but it would be even better if they would be consistent and "dwim".

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 8, 2015

Ok, we talked about this at the libs triage meeting today and the conclusion was to move this into its final comment period.

Another possible idea that was floated was to deprecate str::lines_any, change BufRead::lines and str::lines to handle CRLF, and recommend users of lines move to split. This would be a semantically breaking change, however, and would require an RFC.

@RalfJung

This comment has been minimized.

Copy link
Member Author

RalfJung commented Jul 8, 2015

Does this mean the RFC should be formulated within that one week of final comment period, or that you will decide in one week whether to merge this, or to ask someone to write an RFC?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 8, 2015

Ah this means that we're placing this specific RFC into the final comment period so we'll decide on this next week. If, however, a different strategy is preferred, then this will be closed in favor of an RFC.

@RalfJung

This comment has been minimized.

Copy link
Member Author

RalfJung commented Jul 8, 2015

Okay.So I'll not (yet) try to figure out how to write an RFC ;-)

I should also mention that some people already voiced their opinion in #26710.

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Jul 8, 2015

@RalfJung to be clear, if you'd prefer to change the behavior of the lines APIs (and deprecate the existing lines_any), the team is open to that, but it would need to be debated community-wide through an RFC since it's a breaking change to stable APIs. None of those on the libs team felt strongly enough to push through an RFC rather than just accepting this PR, but you should feel free to do so if you prefer. I'd be glad to help you with the RFC process.

@liigo

This comment has been minimized.

Copy link
Contributor

liigo commented Jul 9, 2015

@retep998

This comment has been minimized.

Copy link
Member

retep998 commented Jul 9, 2015

I would be strongly in favor of that breaking change in semantics to lines.

@RalfJung

This comment has been minimized.

Copy link
Member Author

RalfJung commented Jul 9, 2015

Sorry, then I misunderstood @alexcrichton . Okay, I will try to draft an RFC, and send the draft to @aturon to make sure I don't violate the process. I may not have the time to do that before the weekend, though.

@RalfJung

This comment has been minimized.

Copy link
Member Author

RalfJung commented Jul 11, 2015

FYI: I drafted an RFC at https://github.com/RalfJung/rfcs/blob/line-endings/text/0000-line-endings.md. I'm currently waiting for some feedback before submitting it.

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Jul 11, 2015

@RalfJung Note that BufRead::read_line provides the whole data of what it has read, unmodified, it just stops reading at \n, and that doesn't need to change. So this method is not like the others, I don't think it needs to be part of the RFC (unless I'm missing something).

@RalfJung

This comment has been minimized.

Copy link
Member Author

RalfJung commented Jul 11, 2015

@bluss Good catch, you are right. I'll edit the document accordingly.

Edit: Done. Thanks!

@RalfJung RalfJung referenced this pull request Jul 15, 2015

Merged

RFC: line-endings #1212

@RalfJung

This comment has been minimized.

Copy link
Member Author

RalfJung commented Jul 15, 2015

I submitted this as RFC 1212: rust-lang/rfcs#1212.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 16, 2015

The consensus of the libs team this week was to close this PR in favor of the RFC (as this is listed as an alternative). We can always reopen if that's the decision of the RFC!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.