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

Fix IO::Memory#gets_to_end to consume the IO #4415

Merged

Conversation

jhass
Copy link
Member

@jhass jhass commented May 14, 2017

No description provided.

@@ -225,7 +225,9 @@ class IO::Memory
if pos == @bytesize
""
else
String.new(@buffer + @pos, @bytesize - @pos)
String.new(@buffer + @pos, @bytesize - @pos).tap {
Copy link
Contributor

@RX14 RX14 May 14, 2017

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?

Copy link
Member Author

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.

Copy link
Contributor

@RX14 RX14 May 14, 2017

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.

Copy link
Member Author

@jhass jhass May 14, 2017

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.

Copy link
Contributor

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.

@asterite asterite added this to the Next milestone May 20, 2017
@asterite
Copy link
Member

@jhass Thanks!

@asterite asterite merged commit b2def93 into crystal-lang:master May 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants