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

WIP: Fix unexpected responses from the server #4093

Merged
merged 1 commit into from Apr 23, 2018

Conversation

Projects
None yet
2 participants
@laughedelic
Copy link
Member

commented Apr 11, 2018

Addresses #4091.

85ab082 should fix the first problem.

And I think the second one sits here:

case _ =>
channels collect {
case c: ConsoleChannel =>
c.publishEvent(event)
case c: NetworkChannel =>
try {
c.publishEvent(event)

But I'm not sure how to fix it. In this case it matches any events (so we cannot check channel name here) and redirects them to the NetworkChannel.publishEvent which already doesn't check if the channel name matches.

On the other hand there is also publishEventMessage method which handles ExecStatusEvent and checks channel name. So probably we could reuse it here.

I'm not sure what is the general intent of this piece of code quoted above: if it's not a StringMessage, just broadcast is on all channels? Even if we want to do that, publishEvent is not a good choice, probably using langNotify or something similar would be better.

@laughedelic laughedelic force-pushed the laughedelic:unexpected-responses branch from 85ab082 to a1e3146 Apr 12, 2018

@dwijnand dwijnand added this to the 1.1.5 milestone Apr 23, 2018

@dwijnand dwijnand merged commit cd7eb95 into sbt:1.1.x Apr 23, 2018

3 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dwijnand

This comment has been minimized.

Copy link
Member

commented Apr 23, 2018

sadly I can't help much with the other part, because the general intent is not clear to me either.

@laughedelic laughedelic deleted the laughedelic:unexpected-responses branch Apr 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.