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

kad: rename to follow naming convention across repository #4485

Closed
Tracked by #2217
thomaseizinger opened this issue Sep 12, 2023 · 3 comments · Fixed by #4547
Closed
Tracked by #2217

kad: rename to follow naming convention across repository #4485

thomaseizinger opened this issue Sep 12, 2023 · 3 comments · Fixed by #4547

Comments

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Sep 12, 2023

As tracked in #2217, we should rename the Kademlia behaviour to Behaviour and all related symbols such that users can import it as:

use libp2p::kad;

let b = kad::Behaviour::new(kad::Config::default());
@PanGan21
Copy link
Contributor

PanGan21 commented Sep 23, 2023

Hi @thomaseizinger ! I am interested to contribute this issue. Just one question.
rename the Kademlia behaviour to Behaviour and all related symbols apart from Kademlia -> Behaviour does it mean also to rename KademliaBucketInserts -> BucketInserts, KademliaStoreInserts -> StoreInserts, KademliaConfig -> Config and KademliaCaching -> Caching ?

@thomaseizinger
Copy link
Contributor Author

Hi @thomaseizinger ! I am interested to contribute this issue. Just one question. rename the Kademlia behaviour to Behaviour and all related symbols apart from Kademlia -> Behaviour does it mean also to rename KademliaBucketInserts -> BucketInserts, KademliaStoreInserts -> StoreInserts, KademliaConfig -> Config and KademliaCaching -> Caching ?

In general, yes!

The goal we want to achieve is a balance between symbol length, expressiveness and ambiguity.

The idea is that users can import the libp2p::kad module and then refer to symbols as kad::Config, kad::Behaviour. I'd suggest to think about it like that and rename symbols such that the combined us with the kad:: prefix makes sense!

To start with, dropping the prefix from all symbols should be good enough. As a 2nd step, I'd recommend going over them and checking, that they still make sense. Depending on how thorough you want to be with that this might require diving into understanding what the protocol does (not sure if you are familiar with Kademlia at all?).

Let us know what your appetite is for working on this and we can set the scope accordingly :)

@thomaseizinger
Copy link
Contributor Author

Note: We want this to be backwards compatible. I'd suggest reading some of the conversations in PRs linked in #2217.

@mergify mergify bot closed this as completed in #4547 Sep 27, 2023
mergify bot pushed a commit that referenced this issue Sep 27, 2023
Renamed the following

`kad::Kademlia` -> `kad::Behaviour`
`kad::KademliaEvent` -> `kad::Event`
`kad::KademliaBucketInserts` -> `kad::BucketInserts`
`kad::KademliaStoreInserts` -> `kad::StoreInserts`
`kad::KademliaConfig` -> `kad::Config`
`kad::KademliaCaching` -> `kad::Caching`
`kad::KademliaEvent` -> `kad::Event`
`kad::KademliaConnectionType` -> `kad::ConnectionType`
`KademliaHandler` -> `Handler`
`KademliaHandlerEvent` -> `HandlerEvent`
`KademliaProtocolConfig` -> `ProtocolConfig`
`KademliaHandlerIn` -> `HandlerIn`
`KademliaRequestId` -> `RequestId`
`KademliaHandlerQueryErr` -> `HandlerQueryErr`

Resolves: #4485

Pull-Request: #4547.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants