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

Fix handling id in jsonrpc model #4025

Merged
merged 3 commits into from Mar 18, 2018

Conversation

Projects
None yet
3 participants
@tiqwab
Copy link
Contributor

commented Mar 17, 2018

Workaround for #3861.

This pull request fixes the behavior of sbt server to respond with number or string id judging by the type of id in requests. As suggested by @eed3si9n in ScalaMatsuri hackason, I append a prefix if id in requests is number and use it later to serialize responses.

@eed3si9n eed3si9n added the ready label Mar 17, 2018

@dwijnand dwijnand added the hackathon label Mar 17, 2018

@dwijnand
Copy link
Member

left a comment

♨️!

quite the hack, but it does work around the issue!

do you want to try writing a test for this? it would be great to have something that enforces this new behaviour.

@tiqwab

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2018

I actually tried to write a test in sbt.ServerSpec, but had difficulty handling setup and teardown of sbt server properly for each test case...
Or is it enough in this case to add json serialization/deserialization test? If so, I'll add it.

@eed3si9n

This comment has been minimized.

Copy link
Member

commented Mar 17, 2018

I just sent a PR here tiqwab#1 with a test.

Merge pull request #1 from eed3si9n/wip/fix-parsing-id
Add test case for number id in JSON-RPC
@eed3si9n
Copy link
Member

left a comment

LGTM

@dwijnand dwijnand added this to the 1.1.2 milestone Mar 18, 2018

@dwijnand dwijnand merged commit c6df309 into sbt:1.1.x Mar 18, 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 dwijnand removed the ready label Mar 18, 2018

@dwijnand

This comment has been minimized.

Copy link
Member

commented Mar 18, 2018

thank you @tiqwab!

@tiqwab

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2018

Thank you for your review and help!

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.