-
Notifications
You must be signed in to change notification settings - Fork 247
Use msgpack for API #908
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
Use msgpack for API #908
Conversation
|
I'm not sure what's the best way to go about fixing this. Ideally we also account for it types that have multiple numbers like |
|
I just pushed a commit to account for |
src/snapshot/patch_compute.rs
Outdated
| } | ||
|
|
||
| /// Trait where NaN values must not be treated as different. | ||
| trait Different { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole trait is... not ideal. I would strongly prefer we move the equality check for Variant to its own file, and make it a simple function instead of a trait.
I have a function already implemented in my syncback fork here if you want to steal it.
It does require us to pull in float_cmp as a crate, but I think that's fine.
|
I'm now using the |
Dekkonot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still has the added Different trait despite it not being used for anything anymore. It should be removed.
|
That's my bad, I thought I removed it. |
Dekkonot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no obvious problems with this change, but I'm scared of deploying it since it's not battle tested. msgpack-luau is reasonably well tested and used by Argon so I trust it, but I still worry.
@kennethloeffler What do you think?
| // Ignore images in msgpack-luau because they aren't UTF-8 encoded. | ||
| if file_name.ends_with(".png") { | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could probably stand to be made more robust rather than special-casing png files, but I'm also willing to push that off into a different PR since this is primarily plugin-facing.
|
Because we have urgent changes in the pipeline (namely, support for the new The big thing we'll have to tackle here is more comprehensive testing. I'd looove to have automated tests for this - it's probably something we can feasibly accomplish with Lune! |
|
I would like to revisit this after #1142 merges- as we're already doing a breaking change protocol version bump, it may be good to include this improvement as well |
|
Closing in favor of #1176. |
Fixes #363.
Fixes #881.
Remaining Tasks
msgpackforwriteapi request.NaNinside of patch compute.