Skip to content

Conversation

@dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Mar 1, 2021

If a type param is used in a compact field, don't add TypeInfo + 'static bound (just 'static)
If a type param is used in a compact field, call meta_type::<<$ty as HasCompact>::Type>() instead of meta_type::<$ty>() in the implementation of TypeInfo.

Avoid adding duplicate bounds.

@dvdplm dvdplm marked this pull request as ready for review March 1, 2021 11:02
@dvdplm
Copy link
Contributor Author

dvdplm commented Mar 1, 2021

@ascjones CI is angry about funty. Would be good to make up our minds about this horrible fix.

@ascjones
Copy link
Contributor

ascjones commented Mar 1, 2021

In the absence of better solutions, let's just add that fix to the CI script with a small PR into master so we don't need to do it in each PR. We can remove it if/when we update parity-scale-codec to a version which fixes the issue.

Copy link
Contributor

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

arg my review never got sent ...

@Robbepop
Copy link
Contributor

Robbepop commented Mar 4, 2021

How about also implement manual custom trait bounds for derived code to resolve this problem?

@dvdplm
Copy link
Contributor Author

dvdplm commented Mar 4, 2021

How about also implement manual custom trait bounds for derived code to resolve this problem?

I think we should make the derive as easy to use as possible. Adding these bounds manually requires users to have a firm grasp of both this crate and parity-scale-info's Compact machinery, which imo is a bit unfortunate. Adding manual trait bounds should be a "last resort" option and normal usage should not require it. This is just a personal opinion and it could very well be that the line should be drawn somewhere else as to what is "too advanced" and what isn't.

@Robbepop
Copy link
Contributor

I think this can be closed in favor of our custom trait bounds implementation, right?

@ascjones
Copy link
Contributor

I think this can be closed in favor of our custom trait bounds implementation, right?

It looks like it, @dvdplm can close if he agrees.

@dvdplm
Copy link
Contributor Author

dvdplm commented Jun 21, 2021

Closing as stale.

@dvdplm dvdplm closed this Jun 21, 2021
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