Skip to content

Rename traits to remove T suffix#1535

Merged
jsdw merged 3 commits intomasterfrom
jsdw-no-t-trait-suffix
Apr 16, 2024
Merged

Rename traits to remove T suffix#1535
jsdw merged 3 commits intomasterfrom
jsdw-no-t-trait-suffix

Conversation

@jsdw
Copy link
Copy Markdown
Collaborator

@jsdw jsdw commented Apr 15, 2024

In the end I went for a pattern like:

mod            trait    concrete type              static type        dynamic type
--------------------------------------------------------------------------------------
constants      Address  DefaultAddress<ReturnTy>   StaticAddress<..>  DynamicAddress   
custom_values  Address  -                          StaticAddress<..>  str
storage        Address  DefaultAddress<K,R,F,D,I>  StaticAddress<..>  DynamicAddress<K>
runtime_api    Payload  DefaultPayload<Args,Ret>   StaticPayload<..>  DynamicPayload     
tx             Payload  DefaultPayload<CallData>   StaticPayload<..>  DynamicPayload

Where the static and dynamic types are mostly just aliases to the DefaultX concrete type.

@jsdw jsdw requested a review from a team as a code owner April 15, 2024 15:47
where
Call: PayloadT,
Call: Payload,
Signer: SignerT<T>,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this is fine, just thinking out loud, how would the following look?

  • import crate::tx::Signer (not as Signer T)
  • Use <Call, Sign> where Sing: Signer<T>?

But yea, we don't have to do it since we already have an OfflineClientT and OnlineClientT to distinguish between concret impl and traits

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ohyeah, we do use the T pattern for OfflineClient and OnlineClient too don't we; I guess we are still not super consistent hehe. At least those traits are generally not supposed to be imported/used I guess!

Use <Call, Sign> where Sing: Signer?

I think that makes sense!

Copy link
Copy Markdown
Collaborator

@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! Nice one! 👍

@jsdw jsdw merged commit ac606cf into master Apr 16, 2024
@jsdw jsdw deleted the jsdw-no-t-trait-suffix branch April 16, 2024 15:35
@jsdw jsdw mentioned this pull request May 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