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

DAGJson support for Ipld #390

Merged
merged 22 commits into from
May 6, 2020
Merged

DAGJson support for Ipld #390

merged 22 commits into from
May 6, 2020

Conversation

austinabell
Copy link
Contributor

@austinabell austinabell commented May 2, 2020

Summary of changes
Changes introduced in this pull request:

  • Useful going forward for selectors and testing, implementation is not efficient since the serialization and deserialization requires some theoretically unnecessary clones/copies, but it doesn't matter because this is just JSON and I don't want to fork serde_json or reimplement
    • Added json support for Cids with this (for obvious reasons) but did not for anything outside of IPLD because I don't think it'll be necessary yet
  • Updates serialization tests to use Cid json and cleaned up a few other small things

Also added the ipld! macro for fun (makes things a lot easier)

Reference issue to close (if applicable)

Closes

Other information and links

@austinabell
Copy link
Contributor Author

So pr is blocked by breaking changes in rust-fil-proofs being updated, but also could be some functional changes depending on what happens with ipld/specs#259

Isn't a big rush to get this in so can probably wait for that to resolve

///
/// ```
/// # use forest_ipld::ipld;
/// let v = ipld!(null);
Copy link
Member

Choose a reason for hiding this comment

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

Feels weird having a doc test for how to use an Enum... Just adds to test compilation and running without much value. Your documentation of the macro is pretty good already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made more sense to me to document how to construct each variant instead of doing all on the Ipld enum. These are pretty lightweight but I can add no_run to them or remove, just nice to have examples for each variant for practical use imo.

@@ -44,7 +117,10 @@ pub fn from_ipld<T>(value: &Ipld) -> Result<T, String>
where
T: DeserializeOwned,
{
// TODO find a way to convert without going through byte buffer
// TODO update to not go through byte buffer to convert
Copy link
Member

Choose a reason for hiding this comment

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

This is so janky its funny. But agree we dont need to fix this yet haha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, it's not THAT janky, not like it's a common concept to be able to go from a generalized type to a specific one without a buffer usually. This is correct, just not efficient

Copy link
Member

Choose a reason for hiding this comment

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

Definitely correct, not questioning. I just think its funny how we have to go to CBOR first then deserialize. Anyways, nothing needs to be fixed here.

@austinabell austinabell merged commit 18d179b into master May 6, 2020
@austinabell austinabell deleted the austin/ipld/json branch May 6, 2020 19:31
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.

3 participants