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

Msgpack commands #12664

Merged
merged 15 commits into from
Apr 26, 2024
Merged

Msgpack commands #12664

merged 15 commits into from
Apr 26, 2024

Conversation

devyn
Copy link
Contributor

@devyn devyn commented Apr 25, 2024

Description

I thought about bringing nu_plugin_msgpack in, but that is MPL with a clause that prevents other licenses, so rather than adapt that code I decided to take a crack at just doing it straight from rmp to Value without any rmpv in the middle. It seems like it's probably faster, though I can't say for sure how much with the plugin overhead.

@IanManske I started on a Read implementation for RawStream but just specialized to from msgpack here, but I'm thinking after release maybe we can polish it up and make it a real one. It works!

User-Facing Changes

New commands:

  • from msgpack
  • from msgpackz
  • to msgpack
  • to msgpackz

Tests + Formatting

Pretty thorough tests added for the format deserialization, with a roundtrip for serialization. Some example tests too for both from msgpack and to msgpack.

  • 🟢 toolkit fmt
  • 🟢 toolkit clippy
  • 🟢 toolkit test
  • 🟢 toolkit test stdlib

After Submitting

  • update release notes

@devyn devyn marked this pull request as ready for review April 26, 2024 01:24
@devyn
Copy link
Contributor Author

devyn commented Apr 26, 2024

I did a few last minute things here:

  • from msgpack --objects, like from json --objects, with no delimiter. Just keeps reading any valid msgpack data in a stream. I thought this could be handy for inspecting dumps of the plugin protocol since that's basically what it does
  • without --objects, checks to ensure we actually reached EOF. this is important in case the data is corrupt or we don't handle it properly, because we don't want to return success in that case.
  • moved the tests to nu-command, where they should be

There is something kind of unusual I've done where I'm generating the fixtures with a nu script. In order to prevent anything obfuscated from sneaking in, I'm running that script for each test, but this does obviously depend on nu working correctly, which maybe isn't the best thing for a test to depend on.

However, I tried writing the alternative in Rust with all of the comments and it was just horrible, very hard to follow. Nu's binary syntax makes it a lot better.

I could just check in those MessagePack fixtures already generated as binary instead, I just worry that if someone goes to update it and then the script returns something different than before in some other random fixture, it's going to be very confusing.

@fdncred fdncred added the pr:plugins This PR is related to plugins label Apr 26, 2024
@fdncred fdncred merged commit adf38c7 into nushell:main Apr 26, 2024
16 checks passed
@fdncred
Copy link
Collaborator

fdncred commented Apr 26, 2024

let's go! thanks!

@hustcer hustcer added this to the v0.93.0 milestone Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:plugins This PR is related to plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants