refactor: define trait HashOutput for some Hash associate type
#14220
Conversation
bkchr
added
B0-silent
Co-authored-by: Bastian Köcher <[email protected]>
|
@bkchr Hi, maybe it's still a draft. Maybe should remove all I also think should we Define a super |
yjhmelody
force-pushed
the
refactor-some-traits
branch
from
c15364c to
b370d9d
Compare
There was a problem hiding this comment.
I also think should we Define a super BlockNumber trait. It could reduce so many type constraints code.
👍 For my part I generally like grouping trait constraint, it can result in a few unneeded constraint here or there (having two variant: hash at trie lower level and one at substrate level is already enough probably). And things get more readable.
|
|
||
| impl InspectCmd { | ||
| /// Run the inspect command, passing the inspector. | ||
| pub fn run<B, RA, D>(&self, config: Configuration) -> Result<()> | ||
| where | ||
| B: Block, | ||
| B::Hash: FromStr, |
There was a problem hiding this comment.
Was it the case that this trait bound is only required in situations where std is enabled?
There was a problem hiding this comment.
Yeah, there is MaybeFromStr trait which is parent of HashOutput.
There was a problem hiding this comment.
I think you answered a different question: I was asking if it was required in situations where std is enabled, i.e. before you introduced MaybeFromStr, not whether it is the case right now.
There was a problem hiding this comment.
You know that the runtime does not encourage the use of the String related type. So sp_std have not exported str mod.
There was a problem hiding this comment.
But I personally can't understand why this is done, it may be to prevent problems such as code bloat or utf-8 encoding problems
There was a problem hiding this comment.
I don't think my intention is getting across -- you changed the trait bounds on this type parameter, and hence the API as well. Previously, FromStr is required to be implemented on the Hasher associated type no matter what, but now it's been changed so that it will only be required when you have std enabled.
|
@KiChjang Hi any other problem? |
| @@ -715,6 +703,47 @@ pub trait Hash: | |||
| fn trie_root(input: Vec<(Vec<u8>, Vec<u8>)>, state_version: StateVersion) -> Self::Output; | |||
| } | |||
|
|
|||
| /// Super trait with all the attributes for a hashing output. | |||
| pub trait HashOutput: | |||
There was a problem hiding this comment.
While just a nitpick about the name that has been used. To me would be more correct to generally stick with one naming convention.
That is, seems there is some confusion in the naming convention adopted for hashers, hashes, and hash outputs.
sp-corere-exportshash_db::HasherasHasher.- We have this
Hashtrait here that technically is defining a hasher. - We have
HashOutputtrait here that instrad is theHash.
Can't we rename, just to be coherent these two traits?
Hash -> Hesher
HashOutput -> Hash
This would follow the convention adopted by the standard lib as well
There was a problem hiding this comment.
Personally I would also favor this, but this will for sure result in some very big diff :P
There was a problem hiding this comment.
I agree with you. Maybe you could rename them after this PR merged.
There was a problem hiding this comment.
BTW, there still is Hashing which should be Hasher?
|
Please give me an approve :), ty. |
|
@bkchr Hi, could you merge it? |
|
bot merge |
I think some associate type is too verbose to be used. e.g this one
type Hash: ....So it will reduce so much code for downstream to use it if define a new trait for it.