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

Make buffered.Reader.append accept any ByteSeq. #1644

Merged
merged 1 commit into from
Mar 15, 2017

Conversation

jemc
Copy link
Member

@jemc jemc commented Mar 8, 2017

This is an issue of principle-of-least-surprise, since buffered.Writer
emits ByteSeqs in its output, so having buffered.Reader accept ByteSeqs
at its input makes it symmetrical and least surprising.

This doesn't break any user code, since it only makes an argument type
more general, and does not make any return types more general.

This came up in a discussion in #1639.

@jemc jemc added the changelog - changed Automatically add "Changed" CHANGELOG entry on merge label Mar 8, 2017
@aturley
Copy link
Member

aturley commented Mar 8, 2017

@jemc looks like this doesn't quite work. Let me know if you want to talk about the options.

I like the idea of applying the principle-of-least-surprise, but I also think that the example in #1639 was pretty contrived so it shouldn't be the main driver of what we're doing here.

@jemc
Copy link
Member Author

jemc commented Mar 8, 2017

Odd, I didn't see those failures locally - something must have been screwy with my local environment, since those are legitimate failures.

This is an issue of principle-of-least-surprise, since buffered.Writer
emits ByteSeqs in its output, so having buffered.Reader accept ByteSeqs
at its input makes it symmetrical and least surprising.

This doesn't break any user code, since it only makes an argument type
more general, and does not make any return types more general.
@jemc jemc force-pushed the fix/reader-append-byteseq branch from 4e17826 to dfb9914 Compare March 8, 2017 17:10
@jemc
Copy link
Member Author

jemc commented Mar 8, 2017

Amended to take a slightly different approach in dfb9914.

@jemc
Copy link
Member Author

jemc commented Mar 8, 2017

@Praetonus mentioned to me that we might also want to consider making buffered.Writer return an Array[Array[U8]] instead of an Array[ByteSeq]. Unlike this PR, that would be a breaking change, and would potentially incur a bit of small overhead for the Writer.write and Writer.writev methods if they still accepted a ByteSeq and had to convert to an Array[U8] internally. Also, due to that change being more invasive it would probably be best to do it through RFC.

Still, I think it's at least worth considering that idea before merging this one.

@SeanTAllen SeanTAllen force-pushed the master branch 6 times, most recently from 97b1943 to 21d37aa Compare March 11, 2017 14:51
@sylvanc sylvanc merged commit 6aa9fe9 into master Mar 15, 2017
ponylang-main added a commit that referenced this pull request Mar 15, 2017
@jemc jemc mentioned this pull request Mar 15, 2017
@SeanTAllen SeanTAllen deleted the fix/reader-append-byteseq branch April 6, 2017 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - changed Automatically add "Changed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants