-
Notifications
You must be signed in to change notification settings - Fork 71
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
POC implementation of WAComboResponse acting directly on a socket stream #1240
base: master
Are you sure you want to change the base?
POC implementation of WAComboResponse acting directly on a socket stream #1240
Conversation
611c4f2
to
19b44f4
Compare
@jbrichau I have |
87e1aad
to
5870946
Compare
^ super response ifNil: [ | ||
response := WAComboResponse | ||
onStream: (GRPlatform current writeCharacterStreamOn: (ByteArray new: 1024)) |
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.
This doesn't look right. #writeCharacterStreamOn:
wants a String
, should we instead send #writeCharacterStreamOn:
?
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.
It should be something else, yes. I presume, you meant to write writeBinaryStreamOn:
?
We have to be careful though. I remember having to fight a lot with the wrapping streams (i.e., Zinc).
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 presume, you meant to write writeBinaryStreamOn:?
Yes
I remember having to fight a lot with the wrapping streams (i.e., Zinc).
Let's figure out the problem and solve it.
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.
#writeCharacterStreamOn:
simply delegates to WriteStream on:
, which operates on collections, so in that sense it's fine. Also, this is in a test and there is no #writeBinaryStreamOn:
so far. I'll just leave it if you don't object.
repository/Seaside-Core.package/WAComboResponse.class/instance/commit.st
Outdated
Show resolved
Hide resolved
I think for 4.0 we should bite the bullet and make the codecs |
LGTM apart from my comments. |
Thanks @marschall. I'll make the changes soon. |
Older versions of Seaside don't have a dedicated ASCII converter. Use UTF-8 instead.
5870946
to
dc76211
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1240 +/- ##
==========================================
+ Coverage 48.67% 48.97% +0.29%
==========================================
Files 8946 8889 -57
Lines 80490 80050 -440
==========================================
+ Hits 39178 39201 +23
+ Misses 41312 40849 -463
☔ View full report in Codecov by Sentry. |
This is a clone of #1159, which was opened against the now removed
develop
branch.See also the discussion in #1159.