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

Serialize rationals as floats #76

Closed
wants to merge 1 commit into from
Closed

Conversation

notmgsk
Copy link
Contributor

@notmgsk notmgsk commented Jun 21, 2019

Deserializing also produces floats.

Closes #23.

Deserializing also produces floats.
@notmgsk notmgsk requested a review from a team as a code owner June 21, 2019 01:51
@ecpeterson
Copy link
Contributor

I have mixed feelings about this. We have plans to convert many of our timestamp values from floats to rationals, and so we ultimately want these values to be serialized distinctly. This PR would make us temporarily(?) permissive about silently converting everything to floats, which could lead to headache when we split the types apart. On the other hand, it’s a headache now when you try to serialize a rational and it throws up its hands if you aren’t explicit about the conversion—it’d be best if the serializer knew the type it was supposed to emit, based on the message spec, and converted to float when appropriate.

@@ -95,6 +95,9 @@ The input strings are assumed to be FORMAT-compatible, so sequences like ~<newli
(defmethod %serialize ((payload string))
payload)

(defmethod %serialize ((payload rational))
Copy link
Contributor

Choose a reason for hiding this comment

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

What about specializing on ratio instead of rational?

@notmgsk
Copy link
Contributor Author

notmgsk commented Jun 21, 2019

I can see two ways to do this, since messagepack doesn't natively support a rational type:

  • Serialize it as a hash-table with a "_type" field, and implement a %deserialize-struct that specialises on something like rpcq::|Rational|. This would piggy-back on pre-existing code and require minimal changes.
  • Continue my PR that gives cl-messagepack true extensions. Requires more work / not garaunteed to be merged any time soon, but would be unblock other work (e.g. adding RPCQ support to QVM).

@notmgsk notmgsk closed this Jul 29, 2019
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.

Lisp RPCQ balks on serializing rationals living in float slots
3 participants