Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proto: Rename DomainType trait to Protobuf #672

Merged
merged 9 commits into from
Nov 17, 2020

Conversation

thanethomson
Copy link
Contributor

@thanethomson thanethomson commented Nov 12, 2020

As discussed on our call yesterday, and towards fulfilling #654, in this PR I've:

  1. Renamed the DomainType trait to Protobuf to clarify its purpose.
  2. Kept it in the tendermint-proto crate, because it makes the most sense there.
  3. Added a testable example of how to make use of the trait.
  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG.md

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson thanethomson marked this pull request as ready for review November 12, 2020 15:24
@codecov-io
Copy link

codecov-io commented Nov 12, 2020

Codecov Report

Merging #672 (e35f730) into master (fd3efe4) will not change coverage.
The diff coverage is 78.2%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #672   +/-   ##
======================================
  Coverage    39.6%   39.6%           
======================================
  Files         183     182    -1     
  Lines       12873   12873           
  Branches     2944    2944           
======================================
  Hits         5104    5104           
  Misses       7522    7522           
  Partials      247     247           
Impacted Files Coverage Δ
proto/src/error.rs 0.0% <ø> (ø)
tendermint/src/account.rs 80.2% <ø> (ø)
tendermint/src/block.rs 15.5% <ø> (ø)
tendermint/src/block/header.rs 89.0% <ø> (ø)
tendermint/src/block/height.rs 67.2% <ø> (ø)
tendermint/src/block/id.rs 73.6% <ø> (ø)
tendermint/src/block/parts.rs 68.5% <ø> (ø)
tendermint/src/block/size.rs 0.0% <ø> (ø)
tendermint/src/chain/id.rs 70.7% <ø> (ø)
tendermint/src/consensus/params.rs 0.0% <ø> (ø)
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd3efe4...e35f730. Read the comment docs.

greg-szabo
greg-szabo previously approved these changes Nov 13, 2020
Copy link
Member

@greg-szabo greg-szabo left a comment

Choose a reason for hiding this comment

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

Looks good. Maybe we should also rename the cases where there are multiple Error crates imported and the one from tendermint_proto::Error is aliased as DomainTypeError. A quick search showed me this in vote.rs, proposal.rs, sign_proposal.rs and sign_vote.rs.

tendermint/src/proposal.rs Outdated Show resolved Hide resolved
proto/src/lib.rs Show resolved Hide resolved
proto/src/lib.rs Outdated Show resolved Hide resolved
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
greg-szabo
greg-szabo previously approved these changes Nov 14, 2020
Copy link
Member

@greg-szabo greg-szabo left a comment

Choose a reason for hiding this comment

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

Good stuff.

I'm not sure if the word "equivalent" is the best to describe the similarities between the Protobuf::encode() and the Prost::Message::encode() functions. They have similar functionality that they implement differently. Maybe the word is part of some kind of documentation standard (you seem to have used the same template in multiple places) and I don't have a good enough vocabulary to give you a better alternative. Maybe "replacement"? Protobuf::encode() replaces the Prost::Message::encode() function with the added features of converting to a domain type. Or something.

If people understand "equivalent", then just ignore me.

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson thanethomson merged commit bfcbf83 into master Nov 17, 2020
@thanethomson thanethomson deleted the protos/rename-domain-type branch November 17, 2020 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants