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

Enable generic type encoding via TypeResolver and remove dependency on scale-info #19

Merged
merged 8 commits into from
Feb 16, 2024

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented Feb 14, 2024

This replaces all uses of scale-info with scale_type_resolver::TypeResolver and scale_type_resolver::visitor's to handle resolving types in the various impls.

@jsdw jsdw marked this pull request as ready for review February 15, 2024 17:31
README.md Outdated Show resolved Hide resolved
uses: actions/checkout@v3

- name: Diff READMEs
run: diff -q README.md scale-encode/README.md
Copy link
Member

Choose a reason for hiding this comment

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

dq: why do we need two readme's?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The one in /scale-encode is the oen that is shown on crates.io for the crate, and the one at root is shown on github! This was the laziest way I could think of to make sure that they were kept in sync :)

Copy link
Member

@niklasad1 niklasad1 Feb 16, 2024

Choose a reason for hiding this comment

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

you could just link to root/github README in the Cargo.toml of the crate but fine :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, probably a symlink would work here to avoid some extra CI minutes :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That might not work because if the readme is outside of the crate it won't be uploaded with it, but I'd have to try it!

Copy link
Collaborator Author

@jsdw jsdw Feb 16, 2024

Choose a reason for hiding this comment

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

Git does handle symlinks ok (as long as the FS supports it) though by the sounds of it (eg https://stackoverflow.com/questions/954560/how-does-git-handle-symbolic-links) (I'd assumed that it wouldn't!), so we could just make one readme be a symlink to the other, but I'm not too bothered either way given the CI check :)

@@ -17,8 +17,8 @@

/*!
`parity-scale-codec` provides an `Encode` trait which allows types to SCALE encode themselves based on their shape.
Copy link
Contributor

Choose a reason for hiding this comment

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

dq: Could we #[include_str(README.md)] here?

Copy link
Collaborator Author

@jsdw jsdw Feb 16, 2024

Choose a reason for hiding this comment

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

We could; I think the only reason I don't do that is because the lib docs benefit from doc links ([Foo]) whereas in the README it's better to remove the []'s!

There's almost certainly a tool somewhere though which takes the lib docs and copies them into a README for you, stripping such links!

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense, the only crate I can think of might be: https://crates.io/crates/docify, something that substrate's been using for pallet docs

type_id: &R::TypeId,
types: &R,
) -> Result<Vec<u8>, Error> {
let mut out = Vec::new();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be beneficial at some point in the future to have a size_hint? To preallocate the memory, ensuring we allocate only once?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we could come up with a way that wasn't too complicated I'd be up for it :) Offhand I'm not sure how!

Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

nice

@jsdw jsdw merged commit 82ab8cd into main Feb 16, 2024
9 checks passed
@jsdw jsdw deleted the jsdw-scale-type-resolver branch February 16, 2024 11:15
@jsdw jsdw mentioned this pull request Feb 16, 2024
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