Skip to content

Conversation

@dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Dec 30, 2020

Extract the utils module into it's own crate, parity-scale-codec-derive-utils, so it can be used by scale-info to avoid duplicating code when handling attributes.

It would have been nicer to just make the utils module public but proc-macro crates can't export anything that isn't a proc macro. :/

This can be backported to the 1.3.* series.

…an be used by `scale-info` to avoid duplicating code.

It would have been nicer to just make the `utils` module public but proc-macro crates can't export anything that isn't a proc macro. :/
@bkchr
Copy link
Member

bkchr commented Dec 30, 2020

I'm not convinced that this is the right way.

Can you please give some information on which functions you exactly require? And how does this work in scale-info? How does it process SCALE attributes?

TLDR: It would be nice to have some more information on what is needed and how it should work.

@dvdplm
Copy link
Contributor Author

dvdplm commented Dec 30, 2020

TLDR: It would be nice to have some more information on what is needed and how it should work.

The context here is paritytech/scale-info#8 and paritytech/scale-info#42. When I started to work on paritytech/scale-info#43 (handle "skip") I stopped and went looking for what the set of attributes actually are and read some code. Realized my own is_compact is essentially a worse version of get_enable_compact() and tried to see if we could avoid duplicating the effort.

@bkchr
Copy link
Member

bkchr commented Dec 30, 2020

Hmm okay.

I see your point, however I'm not really convinced that we should push this stuff as its own crate to crates.io. It mostly contains internal code that we use in the derive macros.

If we want to release this as its own crate, I would propose that we may rename some of these functions. Like get_enable_compact

@dvdplm
Copy link
Contributor Author

dvdplm commented Dec 30, 2020

I see your point, however I'm not really convinced that we should push this stuff as its own crate to crates.io. It mostly contains internal code that we use in the derive macros.

I don't like it much either. Just that I like maintaining more code even less. :/
There is a larger scope code-smell here and it is the entanglement between scale-info and parity-scale-codec. Maybe the real fix here is to merge the two? I'm hesitant to bring it up here because scale-info still has to prove its usefulness in the bigger context of substrate, but if it does work out and becomes more important perhaps we end up with implementing TypeInfo as a "side effect" for any type that is Encode/Decode.

Either way, I'm fine closing this and copy&paste code as needed if you prefer (in that case I'll prep a PR with only the docs and renaming a few methods, like get_enable_compact).

@bkchr
Copy link
Member

bkchr commented Dec 30, 2020

If we want to merge this stuff at some future point anyway, I would vote for copying the code for now. :)

Either way, I'm fine closing this and copy&paste code as needed if you prefer (in that case I'll prep a PR with only the docs and renaming a few methods, like get_enable_compact).

Good idea!

@dvdplm
Copy link
Contributor Author

dvdplm commented Dec 31, 2020

Closing in favour of #233

@dvdplm dvdplm closed this Dec 31, 2020
@bkchr bkchr deleted the dp-extract-derive-utils-to-own-crate branch December 31, 2020 15:04
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