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

Clean up external message protocol #2032

Merged
merged 1 commit into from
Feb 5, 2018
Merged

Clean up external message protocol #2032

merged 1 commit into from
Feb 5, 2018

Conversation

jtfmumm
Copy link
Contributor

@jtfmumm jtfmumm commented Feb 2, 2018

  • Remove unused message types (which were the majority)
    and related code.
  • Standardize all ExternalMsg protocols into the simplest one.
  • Add json encoders and decoders for shrink external messages.
  • Update ExternalMsgEncoder/Decoder to use the simple external
    message protocol with json payloads for shrink messages.

Closes #2031

@jtfmumm
Copy link
Contributor Author

jtfmumm commented Feb 2, 2018

These changes are transparent to users of the ExternalMsgEncoder and ExternalMsgDecoder (and are related to the same unit tests which exercise the related methods), except the removed message types, which weren't being used anywhere.

* Remove unused message types (which were the majority)
  and related code.
* Standardize all ExternalMsg protocols into the simplest one.
* Add json encoders and decoders for shrink external messages.
* Update ExternalMsgEncoder/Decoder to use the simple external
  message protocol with json payloads for shrink messages.
Copy link
Contributor

@slfritchie slfritchie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though for the future I wonder if ShrinkQueryJsonDecoder would handle JSON created by a human or another JSON library.

@jtfmumm
Copy link
Contributor Author

jtfmumm commented Feb 5, 2018

@slfritchie Currently it wouldn't because it doesn't ignore spaces, and really all the JSON decoding/encoding will need improvement. I'll open an issue.

@jtfmumm jtfmumm merged commit 4733934 into master Feb 5, 2018
wallaroolabs-wally added a commit that referenced this pull request Feb 5, 2018
@jtfmumm jtfmumm deleted the external-cleanup branch February 5, 2018 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants