-
-
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
Add flush to many builders #8876
Conversation
@@ -244,7 +245,7 @@ module YAML | |||
end | |||
|
|||
# Writes YAML into the given `IO`. A `YAML::Builder` is yielded to the block. | |||
def self.build(io : IO) | |||
def self.build(io : IO) : Nil |
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.
No io.flush
here?
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.
No, because YAML::Builder
already does that.
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.
Why does YAML::Builder
flush but XML::Builder
doesn't? Shouldn't we make this behave identical?
JSON::Builder#flush
delegates to io.flush
which is another different behaviour.
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 noticed that too. I didn't want to start making changes all over the place. That's why I added flush
when needed.
Also, maybe this PR is not needed...
@@ -49,9 +49,10 @@ class YAML::Builder | |||
|
|||
# Creates a `YAML::Builder` that will write to the given `IO`, | |||
# invokes the block and closes the builder. | |||
def self.new(io : IO) | |||
def self.new(io : IO) : Nil |
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.
General question: Should this method be named build
? It's not a constructor and doesn't return an instance of YAML::Builder
which I'd usually expect from a .new
method.
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.
Yeah, I was surprised by this name too. We could probably rename it, but I'd like to avoid doing that in this PR.
I love the branch name, btw 🤣 |
Possible regression from crystal-lang/crystal#8876
Hmm, I believe I've found out one possible regression: calling |
Related #8712. |
Fixes #8875
Sorry that there are no specs but it's a bit cumbersome to test. Plus it's really easy to see what's going on.