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

MessageExtras to comply with TM2i #422

Closed
QuintinWillison opened this issue Jun 15, 2020 · 5 comments · Fixed by #433
Closed

MessageExtras to comply with TM2i #422

QuintinWillison opened this issue Jun 15, 2020 · 5 comments · Fixed by #433
Assignees

Comments

@QuintinWillison
Copy link
Contributor

Originally specified: ably/ably-java#580

And fixed in: ably/ably-java#581

@QuintinWillison
Copy link
Contributor Author

@sacOO7 it may be that this issue can simply be closed - can you validate if this has been fixed, or create a PR to fix accordingly. Thanks.

@sacOO7
Copy link
Collaborator

sacOO7 commented Aug 1, 2020

Findings with respect to ably/ably-java#581

  • MessageExtras does not check for null data (JToken) in the constructor.
  • DeltaExtras does not check for null Format and From values.
  • ToJson() method can be simplified, data (Jtoken) already contains Delta property.

@sacOO7
Copy link
Collaborator

sacOO7 commented Aug 2, 2020

Note - As per https://docs.ably.io/client-lib-development-guide/features/#TM2i, JsonObject is specified as optional (nullable) value.
Need to confirm if its null or non-null.

@marto83
Copy link
Contributor

marto83 commented Aug 3, 2020

I leave it null because this way we don't serialise an empty json object ({}) when sending the message to the server. Same for From and Format. If the client are never going to set them we can probably make the constructor private.

Agree about ToJson() now everything is immutable we can simplify it.

@sacOO7
Copy link
Collaborator

sacOO7 commented Aug 11, 2020

@QuintinWillison we will need your inputs on this #422 (comment)

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

Successfully merging a pull request may close this issue.

3 participants