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

POC implementation of WAComboResponse acting directly on a socket stream #1159

Conversation

theseion
Copy link
Member

No description provided.

@theseion theseion requested review from marschall and jbrichau August 26, 2019 13:31
@theseion
Copy link
Member Author

theseion commented Aug 26, 2019

I also added this method to GRPlatform:

doesVmSupportStringToByteArrayConversion
	^ doesVmSupportStringToByteArrayConversion ifNil: [
		doesVmSupportStringToByteArrayConversion := [
			(ByteArray new: 1)
				replaceFrom: 1
				to: 1
				with: 'a'.
			true ]
					on: Error
					do: [ false ] ]

And WABinaryConvertingStream.

Tests are failing because of the missing Grease dependency.

@theseion
Copy link
Member Author

See #1145

@marschall
Copy link
Contributor

I also added this method to GRPlatform:

doesVmSupportStringToByteArrayConversion
	"..."

I assume this method would be used to decide whether we need to wrap in a BinaryConvertingStream. I wonder whether we could do without this and instead let either a GRPlatform method or adapter method return the correct type of stream.

And WABinaryConvertingStream.

I assume you mean GRBinaryConvertingStream

@theseion
Copy link
Member Author

@marschall As far as I can tell there are no facilities on GRPlatform yet that deal with differences in VM's, which might be an issue in this case, when there are multiple VM's to choose from for a given platform.
Where would you put the test on the adapter? The problem I see there is that we'd have to pass the adaptor as an argument to the response object that we're creating (see ZnZincStreamingServerAdapter>>responseFor:), which feels wrong to me. It's not a property of the chosen server.

I do agree that it doesn't feel quite right on GRPlatform either. Maybe we need to add something like GRVmFacilities?

@jbrichau jbrichau closed this Feb 14, 2021
@jbrichau jbrichau deleted the branch SeasideSt:develop February 14, 2021 09:44
@jbrichau
Copy link
Member

@theseion I removed the develop branch but did not intend to discard this PR. I know you're on other tracks right now, but would you mind pulling this change against master? I don't know the status and if someone can finish it, but would be good to keep track of it. Thanks!

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

Successfully merging this pull request may close these issues.

3 participants