Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Rename Hashing Trait to Hash #288

Merged
merged 1 commit into from
Jul 10, 2018

Conversation

gnunicorn
Copy link
Contributor

@gnunicorn gnunicorn commented Jul 9, 2018

fixes #172 .

Rename runtimes-primitives::traits::Hashing to Hash to make it more idiomatic. Also replaces HashingFor with HashFor and all the dependent uses of any of them in our code base. The cases I opted for Hash as HashT-imports is because it would otherwise lead to conflicting/overwriting imports from other modules.

Note: there is still one occurrence of Hashing in the code base: as a type on Header, which has both Hash and Hashing types defined. I was thinking of renaming it Hasher or Hashed but it wasn't clear to me what this is really referring to.

@gnunicorn gnunicorn changed the title Rename Hasing Trait to Hash Rename Hashing Trait to Hash Jul 9, 2018
@gnunicorn gnunicorn added the A0-please_review Pull request needs code review. label Jul 9, 2018
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

Naming... sigh. :)

So in a recent refactoring in parity-ethereum a Hasher trait was added with the stated goal of enabling code sharing with Polkadot (e.g. patricia_trie, memorydb and others).
I am honestly not sure what the relationship is to Hash/Hashing but they sure look very alike. Should they be unified perhaps?

@simonljs
Copy link
Contributor

simonljs commented Jul 9, 2018

Hi guys. I was close to committing some stuff myself. I had assigned #172 to myself, but I am new to GitHub - is there something else that I should do in future?

@gnunicorn
Copy link
Contributor Author

@dvdplm not sure. This is very specific change and I am honestly not yet clear enough of all the moving parts to be able to do anything about it.

@simonljs I am sorry, that is my bad, I didn't see you had assigned that to yourself, when starting it. I don't think there is much else you could/should do, as I am not aware we've defined a proper process for project management, including how tracking would work so we don't have multiple people working on the same thing (correct me if I'm wrong). Again, I am sorry for having stepped on your toes here!

@simonljs
Copy link
Contributor

simonljs commented Jul 9, 2018

@gnunicorn That's ok - I'm new to the codebase so it was a useful learning exercise anyway! :)

@rphmeier
Copy link
Contributor

rphmeier commented Jul 9, 2018

@dvdplm yes, the Hasher trait should also be Hash and we should unify them at some point.

@dvdplm
Copy link
Contributor

dvdplm commented Jul 9, 2018

@rphmeier Ok, cool.

@gnunicorn lgtm

@gavofyork gavofyork added A8-looksgood and removed A0-please_review Pull request needs code review. A8-looksgood labels Jul 9, 2018
@gavofyork
Copy link
Member

... although doesn't build?

@gnunicorn
Copy link
Contributor Author

@gavofyork it's my lucky day, looks like pwasm-utils broke (on nightly), independently from this changeset :( .

@pepyakin
Copy link
Contributor

pepyakin commented Jul 9, 2018

fix is here

@gavofyork gavofyork merged commit 9b438dc into paritytech:master Jul 10, 2018
@gnunicorn gnunicorn deleted the ben-rename-hashing-trait branch July 10, 2018 10:07
JoshOrndorff pushed a commit to moonbeam-foundation/substrate that referenced this pull request Apr 21, 2021
Adds trace_filter RPC endpoint, with a background task to keep a cache of the traces to avoid redundant computations.
liuchengxu pushed a commit to chainx-org/substrate that referenced this pull request Aug 23, 2021
liuchengxu added a commit to chainx-org/substrate that referenced this pull request Aug 23, 2021
* Change aura to babe consensus

* .

* cargo fmt

* Add babe rpc

* Nits

* .

* Change epoch duration to 5 minutes

* .
liuchengxu added a commit to subspace/substrate that referenced this pull request Jun 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename Hashing trait to Hash
6 participants