feat: Add support for generics in interface #231
Conversation
Codecov Report
@@ Coverage Diff @@
## feat/generics_support #231 +/- ##
=========================================================
+ Coverage 87.72% 88.24% +0.52%
=========================================================
Files 24 25 +1
Lines 1303 1336 +33
=========================================================
+ Hits 1143 1179 +36
+ Misses 160 157 -3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
jawoznia
force-pushed
the
support_generic_interfaces
branch
from
40c98fd to
5a652ed
Compare
sylvia/tests/generics.rs
Outdated
| for<'msg_de> Msg: Serialize | ||
| + Deserialize<'msg_de> | ||
| + std::fmt::Debug | ||
| + PartialEq | ||
| + Clone | ||
| + schemars::JsonSchema, |
There was a problem hiding this comment.
We cannot expect anyone to list this kind of trait bound. However - isn't it simply cosmwasm_std::CustomMsg?
There was a problem hiding this comment.
Aside of Deserialize yes. It's better to use here CustomMsg with Deserialize to confirm that lifetimes are parsed properly.
There was a problem hiding this comment.
Then DeserializeOwned > Deserialize in the example (simpler, and it is rare you benefit from unowned deserialize).
However - how does CustomMsg not require DeserializeOwned, but is it needed here? O.o Strange, will take a look at that.
There was a problem hiding this comment.
As agreed I will leave one example with Deserialize<'de> just to keep parsing lifetimes tested.
I will also introduce sylvia::types::CustomMsg that will provide bound on both DeserializeOwned and CustomMsg
| @@ -303,7 +303,7 @@ impl<'a> EnumMessage<'a> { | |||
| #[allow(clippy::derive_partial_eq_without_eq)] | |||
| #[derive(#sylvia ::serde::Serialize, #sylvia ::serde::Deserialize, Clone, Debug, PartialEq, #sylvia ::schemars::JsonSchema, cosmwasm_schema::QueryResponses)] | |||
| #[serde(rename_all="snake_case")] | |||
| pub enum #unique_enum_name #generics #where_clause { | |||
There was a problem hiding this comment.
You sure we can get rid of this entirely? I Have a feeling we will bring it back at some form, but maybe refactored.
There was a problem hiding this comment.
I removed it because of duplicated bound on Deserialize impl.
Without the where clause
#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
pub enum MyEnum<Msg> {}We will get no additional bounds here
impl<'de, Msg> _serde::Deserialize<'de> for MyEnum<Msg>
where
Msg: _serde::Deserialize<'de>,
{But if user will provide a Deserialize bound here
#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
pub enum MyEnum<Msg>
where
for<'a> Msg: Deserialize<'a>,Deserialize will expand to
impl<'de, Msg> _serde::Deserialize<'de> for MyEnum<Msg>
where
for<'a> Msg: Deserialize<'a>,
Msg: _serde::Deserialize<'de>,This also aligns with what dtolnay said here serde-rs/json#469 (comment). Basically to keep behavior bounds out of the type and move them to impl block.
There was a problem hiding this comment.
Ok, it makes sense. First time I've seen I didn't yet know you moved it. I will check generated code today.
| generics.as_slice(), | ||
| &unbonded_generics, | ||
| ); | ||
| let where_clause = if !wheres.is_empty() { |
There was a problem hiding this comment.
I've seen you removed qouting the where clause, the question is - why do you even calculate it then?
There was a problem hiding this comment.
Ok, I see, you moved it.
sylvia-derive/src/message.rs
Outdated
| #(#methods_impl)* | ||
| } | ||
|
|
||
|
|
||
| pub trait Querier { | ||
| pub trait #querier #where_clause { |
There was a problem hiding this comment.
Really needed? 90% of cases you don't want to have where clauses on this level. The best is to keep them only on impl level, or on function definition level. Anywhere else is only when there is very good explanation for that.
There was a problem hiding this comment.
You are right. This is not needed
jawoznia
force-pushed
the
support_generic_interfaces
branch
2 times, most recently
from
9a158e0 to
f01f822
Compare
close #224