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.

Sign up for GitHub

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

Jump to bottom

refactor(identity): follow naming conventions for conversion methods #3775

Merged
merged 47 commits into from
Apr 14, 2023
Merged

refactor(identity): follow naming conventions for conversion methods #3775

merged 47 commits into from
Apr 14, 2023

Conversation

drHuangMHT
Copy link
Contributor

@drHuangMHT drHuangMHT commented Apr 12, 2023 •
edited
Loading

Description

This PR renames some method names that don't follow Rust naming conventions or behave differently from what the name suggests:

  • Enforce "try" prefix on all methods that return Result.
  • Enforce "encode" method name for methods that return encoded bytes.
  • Enforce "to_bytes" method name for methods that return raw bytes.
  • Enforce "decode" method name for methods that convert encoded key.
  • Enforce "from_bytes" method name for methods that convert raw bytes.

Notes & open questions

Some method are intentionally left out to prevent conflict with #3681.
Base branch is contaminated by #3621.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

drHuangMHT and others added 29 commits March 13, 2023 19:06
@drHuangMHT
feat: make mdns::Event clone-able
7d5e606
Merge branch 'libp2p:master' into master
05ce438
@drHuangMHT
docs: filling CHANGELOG for PR 3606
a72e5e2 
Unreleased patch version 0.43.1 bumped. Added entry for PR 3606: Deriving 'Clone' for 'mdns::Event'
@drHuangMHT
Merge branch 'master' of https://github.com/drHuangMHT/rust-libp2p
09bef20
@thomaseizinger
Update protocols/mdns/CHANGELOG.md
926c7ca
@thomaseizinger
Update protocols/mdns/CHANGELOG.md
49d26c4
@mergify
Merge branch 'master' into master
4a35406
@drHuangMHT
feat: change 'mdns::Event' to hold 'SmallVec'
a8ff1eb
@drHuangMHT
test: adapting test to 'mdns::Event' change
4cec5fa
@drHuangMHT
Merge branch 'master' of https://github.com/drHuangMHT/rust-libp2p
a876f2f
@drHuangMHT
feat(mdns): change 'mdns::Event' to hold 'Vec'
18811d2
@drHuangMHT
doc: add entry for libp2p#3621
abdce7e 
New unreleased minor version 0.44.0.
@thomaseizinger
Update protocols/mdns/CHANGELOG.md
d5c246c 
Co-authored-by: Thomas Eizinger <[email protected]>
@drHuangMHT
Merge branch 'master' of https://github.com/drHuangMHT/rust-libp2p
ed128a7
@drHuangMHT
Merge branch 'master' of https://github.com/drHuangMHT/rust-libp2p
0d06f4c 
Unsynced co-authered change.
@drHuangMHT
Remove conflict marker to resolve conflicts
9d45cf3
@drHuangMHT
Merge branch 'master' of https://github.com/drHuangMHT/rust-libp2p
752002b
@drHuangMHT
rename methods in 'libp2p-identity::ecdsa'
74d01be
@drHuangMHT
introduce 'KeyType' in 'libp2p-identity'
d219b0e
@drHuangMHT
introduce 'OtherVariantError' in 'libp2p-identity::error'
8ca95ee
@drHuangMHT
rename methods in 'libp2p-identity::ed25519'
9b81b81
@drHuangMHT
rename methods in 'libp2p-identity::keypair'
7a1f645
@drHuangMHT
rename methods in 'libp2p-identity::rsa'
25e1139
@drHuangMHT
rename methods in 'libp2p-identity::secp256k1'
d15d95b
@drHuangMHT
formatting
19ff427
@drHuangMHT
change call to deprecated methods to recommended ones
ba4511d
@drHuangMHT
clippy fix
8e9c227
@drHuangMHT
formatting
9ed6436
@thomaseizinger
Undo mdns changes
cbbd01b
thomaseizinger
thomaseizinger reviewed Apr 12, 2023
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Very nicely done, only a few nits!

Also, can you add a changelog entry to libp2p-identity please and bump the patch version in the manifest of libp2p-identity?

identity/src/ecdsa.rs Outdated Show resolved Hide resolved
identity/src/ecdsa.rs Show resolved Hide resolved
identity/src/ed25519.rs Outdated Show resolved Hide resolved
identity/src/ed25519.rs Outdated Show resolved Hide resolved
identity/src/error.rs Outdated Show resolved Hide resolved
identity/src/error.rs Outdated Show resolved Hide resolved
@drHuangMHT
Copy link
Contributor Author

I've intentionally kept Cargo.lock untouched. Should I commit the change?

@thomaseizinger
Copy link
Contributor

I've intentionally kept Cargo.lock untouched. Should I commit the change?

Yes, the lockfile needs to be up to date.

thomaseizinger
thomaseizinger reviewed Apr 13, 2023
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Nicely done, only one more things!

identity/CHANGELOG.md Outdated Show resolved Hide resolved
identity/src/error.rs Show resolved Hide resolved
@thomaseizinger thomaseizinger changed the title refactor(identity): follow Rust naming conventions for conversion methods refactor(identity): follow naming conventions for conversion methods Apr 14, 2023
@thomaseizinger thomaseizinger mentioned this pull request Apr 14, 2023
Closed
@thomaseizinger thomaseizinger marked this pull request as ready for review April 14, 2023 06:58
thomaseizinger
thomaseizinger approved these changes Apr 14, 2023
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

This is ready for merge. We are just missing some version bumps to point to the correct version of libp2p-identity.

@mxinden We are renaming several functions to be consistent with Rust's naming guidelines. Unfortunately, there is no clippy lint for this.

protocols/gossipsub/src/behaviour.rs Show resolved Hide resolved
protocols/identify/src/protocol.rs Show resolved Hide resolved
transports/noise/src/io/handshake.rs Show resolved Hide resolved
transports/plaintext/src/handshake.rs Show resolved Hide resolved
transports/tls/src/certificate.rs Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor

Awesome, thank you for sending this PR!

@mergify mergify bot merged commit 058c2d8 into libp2p:master Apr 14, 2023
tcoratger pushed a commit to tcoratger/rust-libp2p that referenced this pull request Apr 14, 2023
@drHuangMHT @tcoratger
refactor(identity): follow naming conventions for conversion methods
fc44e0a 
This PR renames some method names that don't follow Rust naming conventions or behave differently from what the name suggests:
- Enforce "try" prefix on all methods that return `Result`.
- Enforce "encode" method name for methods that return encoded bytes.
- Enforce "to_bytes" method name for methods that return raw bytes.
- Enforce "decode" method name for methods that convert encoded key.
- Enforce "from_bytes" method name for methods that convert raw bytes.

Pull-Request: libp2p#3775.
@thomaseizinger thomaseizinger mentioned this pull request May 5, 2023
Merged
3 tasks
mxinden
mxinden reviewed May 8, 2023
View reviewed changes
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Sorry for the late review here. Great work @drHuangMHT!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxinden mxinden mxinden left review comments

@thomaseizinger thomaseizinger thomaseizinger approved these changes

Assignees
No one assigned
Labels
send-it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@drHuangMHT @thomaseizinger @mxinden