test changes #1473
test changes #1473
Conversation
crates/metadata/src/specs.rs
Outdated
| @@ -1035,12 +1035,12 @@ where | |||
| /// | |||
| /// # Important Note | |||
| /// Only use this factory with constructors! | |||
| pub trait TransformType { | |||
| pub trait ConstructorReturnType { | |||
There was a problem hiding this comment.
The name is misleading IMO. It doesn't reflect the functionality of the trait. The whole idea behind this trait is to transform/convert the return type into either () or Result<(), E> . this doesn't necessary mean it can only be applied to constructors. Additionally we already have ConstructorReturnType<C> in ink::codegen::dispatch module. This can also arise confusion in future
There was a problem hiding this comment.
Additionally we already have ConstructorReturnType
Ooh that's my bad. I wasn't aware of this. So I do agree we should change it to something not already present.
The whole idea behind this trait is to transform/convert the return type into either () or Result<(), E>
I can't see the "transforming" part for the trait name, neither the "new" part in the traits method name. Implementing this trait for some type T indicates what the return type spec of this T will be inside the metadata, given T is used as the result of a constructor.
this doesn't necessary mean it can only be applied to constructors.
I can't think of another use case here. Besides that, how would this interface exactly work? It only allows to implement one "transformation" per particular type. How would this be useful e.g. for a type that is returned by a constructor and is also used for something else which needs a conflicting "transformation"? I don't think we should prematurely solve this problem right now.
There was a problem hiding this comment.
Inspired by ConstructorReturnType I renamed the trait to ConstructorReturnSpec. Lines like this look much cleaner to me now, what do you think?
| match ret_ty { | ||
| None => { | ||
| quote! { | ||
| ::ink::metadata::ReturnTypeSpec::new(::core::option::Option::None) | ||
| } | ||
| } | ||
| Some(syn::Type::Path(syn::TypePath { qself: None, path })) |
There was a problem hiding this comment.
Definitely cleaner approach!
Codecov Report
@@ Coverage Diff @@
## gn/constructor-metadata #1473 +/- ##
===========================================================
- Coverage 64.41% 64.41% -0.01%
===========================================================
Files 201 201
Lines 6239 6236 -3
===========================================================
- Hits 4019 4017 -2
+ Misses 2220 2219 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
LGTM! |
* generates correct metadata for Self type * generate metadata for Result type * generate metadata for Result<Self, ...> * docs and comments * cargo fmt * fix the metadata tests * makes compatible with new tests * rewrite return type transformation to support aliases * test changes (#1473) * test changes * rename trait to ConstructorReturnSpec * adapt test * remove unecessary helper fn (#1475) * remove unecessary helper fn * fix some comments * rename var * Update crates/ink/codegen/src/generator/metadata.rs Co-authored-by: Cyrill Leutwiler <cyrill@parity.io> * Add test for metadata generator (#1477) * Add test for metadata generator * nightly fmt Co-authored-by: Cyrill Leutwiler <cyrill@parity.io>
No description provided.