-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix IO::Memory#gets_to_end to consume the IO #4415
Fix IO::Memory#gets_to_end to consume the IO #4415
Conversation
@@ -225,7 +225,9 @@ class IO::Memory | |||
if pos == @bytesize | |||
"" | |||
else | |||
String.new(@buffer + @pos, @bytesize - @pos) | |||
String.new(@buffer + @pos, @bytesize - @pos).tap { |
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.
Isn't the style rule that multi-line blocks are always do
/end
? And should the formatter enforce this?
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.
Uh well my personal style certainly isn't, the formatter doesn't seem to enforce it and https://crystal-lang.org/docs/conventions/coding_style.html doesn't mention anything about it.
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.
The ruby style guide discourages it for the reason that multi-line brace blocks encourage chaining methods on the return values of multi-line blocks. And I tend to agree with it. I'm not sure what the core team thinks though.
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 roughly follow Weirich's rule personally, with the variation of communicating whether I care about the call's return value rather than the block's. Most of the time that matches up but this is one of the cases where it doesn't.
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.
In my opinion, the stdlib should stick to one rule, and that rule should be easy to enforce.
@jhass Thanks! |
No description provided.