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

Upstreams Aptos Labs patches #34

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

gedigi
Copy link

@gedigi gedigi commented Apr 6, 2023

Summary

These changes were made by the Aptos Labs team from a fork of @novifinancial/serde-reflection. Pushing them upstream to remove dependency on forked version.

@gedigi
Copy link
Author

gedigi commented Apr 12, 2023

@ma2bd I'm planning to open this PR but I'm having issues with local testing, mostly reverse-engineering the packages to be installed on the system, and the fact that the .NET versions supported in serde-generate are out of support from Microsoft. Any tips on how to setup a local test environment? Also, should I provide a patch that targets .NET 7, the currently supported version? Thanks!

@ma2bd
Copy link
Contributor

ma2bd commented Apr 25, 2023

@gedigi Thanks for sharing your changes upstream. I think we could update the version of .NET in CI and officially in this repo.

@@ -186,10 +186,6 @@ where
.flatten()
.cloned()
.collect::<HashSet<_>>();
writeln!(self.out, "#![allow(unused_imports)]")?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite understand the motivation for this change. This seems to potentially break user code.

@@ -65,6 +65,22 @@ public string deserialize_str()
return utf8.GetString(content);
}

public ValueArray<ValueArray<byte>> deserialize_vec_bytes() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this meant to be called by a modified version of the code-generator? (that we don't have here)
Why do we need this in the first place?

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.

4 participants