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

Align flush behaviour of builders #8891

Closed
straight-shoota opened this issue Mar 9, 2020 · 2 comments · Fixed by #9321
Closed

Align flush behaviour of builders #8891

straight-shoota opened this issue Mar 9, 2020 · 2 comments · Fixed by #9321

Comments

@straight-shoota
Copy link
Member

Extracted from #8876 (comment)

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.

I didn't look into this any deeper, so I'm not sure what should be the best way. But every builder behaves a bit different. There are also a few more builders in stdlib which should be covered as well.

@straight-shoota
Copy link
Member Author

So there are a few angles to this:

  • Builders typically expose a .build method, either on the builder itself or the parent namespace. Such a build method generates a self-contained piece and it makes sense to flush there.
  • Most builders also have internal methods that mark the end of a self-contained piece, for example YAML::Builder#end_stream. That method seems like it would be a good idea to flush. But what about YAML::Builder#end_document? Should that flush? It's a self contained piece. But in most typical use cases, a stream only contains a single document, so it just duplicates the flush on end_document. Probably not going to make a noticable performance difference, though.

It's probably undisputed that .build methods should make sure to always flush.

But I'm not sure if any of the API methods should flush. Typical use cases should always use the .build helper. If you don't use that it's because you need some special behaviour and I think it makes sense to be responsible for flushing then.
It might still be a good idea for API methods that clearly mark the end of the building sequence (for example after YAML::Builder#end_sequence is called, you can't add anything to the builder).

@straight-shoota
Copy link
Member Author

straight-shoota commented May 19, 2020

Also it seems to make sense to always flush the underlying IO when flush is called on the builder (or any IO). Some builders expose this method when the builder implementation provides it, but it does not (yet) call flush the underlying IO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant