ByteStream consumer can write to interface{}#273
Merged
fredbi merged 1 commit intogo-openapi:masterfrom Dec 12, 2023
Merged
Conversation
Member
Author
|
cc: @danny-cheung |
Member
Author
|
@casualjim @youyuanwu I am interested by your feedback on this approach, as it doesn't make sense to adopt it for just the Bytestream Consumer. Please let me know what are your thoughts. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #273 +/- ##
==========================================
+ Coverage 80.83% 80.99% +0.15%
==========================================
Files 44 44
Lines 3366 3394 +28
==========================================
+ Hits 2721 2749 +28
Misses 535 535
Partials 110 110
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
casualjim
reviewed
Dec 8, 2023
d2cdb7c to
e1af0d1
Compare
casualjim
reviewed
Dec 9, 2023
e1af0d1 to
a7fcf73
Compare
casualjim
approved these changes
Dec 11, 2023
Member
Author
|
@casualjim I'd like to add a little more test work on that one. I'd like to benchmark a bit to figure out the actual savings with not entering the "reflected" code path. If this is proves to be a sound pattern, I'll generalize in a follow-up pattern, e.g. fixing #263 |
a7fcf73 to
e827593
Compare
Member
Author
|
e827593 to
d9f89fe
Compare
Member
Author
|
@casualjim the benchmark demonstrates that I was wrong ... There is actually no material benefit from favoring type a switch over reflect (when done correctly). The benchmark shows that the reflection path is actually quite efficient and that the reflect.Value is not allocated on the heap. Therefore, I am going to simplify this and remove the redundant cases. |
* fix(ByteStreamConsumer): may now write into an interface which underlying type is []byte or string. * feat(ByteStreamConsumer): added support to io.ReaderFrom, preferred over io.Writer if available * feat(ByteStreamProducer): added support to io.WriterTo, preferred over io.Reader if available * refact(ByteStreamProducer): removed redundant case "string" and preferred the more general reflected case (supports aliased strings) * test: refactored ByteStream tests * test: added benchmark for bytestream.Consume * fixes go-openapi#167 Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
d9f89fe to
0a0b04d
Compare
youyuanwu
approved these changes
Dec 12, 2023
fredbi
added a commit
to fredbi/runtime
that referenced
this pull request
Dec 13, 2023
This is a follow-up on go-openapi#273. I realized that a few typos escaped my review on docstrings and that some misnomers for variables in tests made the tests difficult to read (e.g. rdr for a Writer...). Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
fredbi
added a commit
to fredbi/runtime
that referenced
this pull request
Dec 13, 2023
This is a follow-up on go-openapi#273. I realized that a few typos escaped my review on docstrings and that some misnomers for variables in tests made the tests difficult to read (e.g. rdr for a Writer...). Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
fredbi
added a commit
that referenced
this pull request
Dec 13, 2023
This is a follow-up on #273. I realized that a few typos escaped my review on docstrings and that some misnomers for variables in tests made the tests difficult to read (e.g. rdr for a Writer...). Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ByteStream consumer can write to interface{}
fix(ByteStreamConsumer): may now write into an interface which
underlying type is []byte or string.
feat(ByteStreamConsumer): added support to io.ReaderFrom, preferred
over io.Writer if available
feat(ByteStreamProducer): added support to io.WriterTo, preferred
over io.Reader if available
refact(ByteStreamProducer): removed redundant case "string" and preferred
the more general reflected case (supports aliased strings)
test: refactored ByteStream tests
test: added benchmark for bytestream.Consume
fixes ByteStreamConsumer can't write to interface{} #167
Signed-off-by: Frederic BIDON fredbi@yahoo.com