Skip to content

Serialize rationals as floats#76

Closed
notmgsk wants to merge 1 commit intomasterfrom
fix/serialize-rational
Closed

Serialize rationals as floats#76
notmgsk wants to merge 1 commit intomasterfrom
fix/serialize-rational

Conversation

@notmgsk
Copy link
Copy Markdown
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
Copy Markdown
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.

(defmethod %serialize ((payload string))
payload)

(defmethod %serialize ((payload rational))
Copy link
Copy Markdown
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
Copy Markdown
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