refactor(gossipsub): revise symbol naming to follow conventions #3303

StemCll · 2023-01-02T21:03:46Z

Description

Changes regarding the #2217

Notes

Links to any relevant issues

Open Questions

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

mergify · 2023-01-03T19:45:20Z

This pull request has merge conflicts. Could you please resolve them @StemCll? 🙏

StemCll · 2023-01-05T12:07:23Z

I have to test the changes from backwards compatibility correctness, however I think it's ready for review cc @thomaseizinger 🙏

examples/gossipsub-chat.rs

protocols/gossipsub/src/lib.rs

thomaseizinger

Thanks for doing this :)

I'd spot checked it and looks alright. I'll give it a more thorough look once we have the comments resolved!

thomaseizinger · 2023-01-09T01:24:24Z

protocols/gossipsub/src/lib.rs

+#[deprecated(
+    since = "0.44.0",
+    note = "Use re-exports that omit `Gossipsub` prefix, i.e. `libp2p::gossipsub::config::Builder"
+)]
+pub type GossipsubBuilder = config::Builder;


Ah yes. That is a tricky one.

I somewhat agree that config::Builder is more accurate. However, we didn't call it GossipsubConfigBuilder before so if we stick to gossipsub::Builder now we technically aren't making the situation any worse :)

For the sake of not bike-shedding this and moving on, I'd suggest to just stick to that and keep config private.

If somebody cares enough, they can think of a solution to improve this assuming balanced trade-offs (private/public modules, length of type names/paths, expressiveness, etc).

protocols/gossipsub/src/lib.rs

thomaseizinger

Thanks! This looks pretty good already.

I've made some inline comments, please check for yourself that we are only renaming actual types and not comments.

Another thing we've noticed in previous PRs for renaming types is that the rustdoc comments might suddenly read a bit odd if it f.e. just says Behaviour now instead of Gossipsub. I've not through all docs in this review yet, can you please check them?

Some of the deprecation type aliases are still missing. I can recommend to temporarily undo all changes in examples/ and tests/ and see if everything still compiles. If not, you are missing a deprecation type alias :)

thomaseizinger · 2023-01-11T03:36:55Z

protocols/gossipsub/src/types.rs

@@ -23,7 +23,7 @@ use crate::rpc_proto;
 use crate::TopicHash;
 use libp2p_core::{connection::ConnectionId, PeerId};
 use prometheus_client::encoding::EncodeLabelValue;
-use prost::Message;
+use prost::Message as ProtobufMessage;


If you just need to the trait to be in scope to call functions on it you can do this:

Suggested change

use prost::Message as ProtobufMessage;

use prost::Message as _;

Fixed all occurrences in gossipsub

thomaseizinger · 2023-01-11T03:48:39Z

protocols/gossipsub/src/behaviour.rs

@@ -670,7 +667,7 @@ where
                    }
                }

-                // Gossipsub peers
+                // Behaviour peers


This rename doesn't make much sense IMO.

thomaseizinger · 2023-01-11T03:48:52Z

protocols/gossipsub/src/behaviour.rs

@@ -919,7 +916,7 @@ where
        }
    }

-    /// Gossipsub JOIN(topic) - adds topic peers to mesh and sends them GRAFT messages.
+    /// Behaviour JOIN(topic) - adds topic peers to mesh and sends them GRAFT messages.


Same here. Can you revert the ones in the comments please?

thomaseizinger · 2023-01-11T03:53:02Z

protocols/gossipsub/src/error.rs

@@ -88,7 +88,7 @@ impl From<SigningError> for PublishError {

 /// Errors that can occur in the protocols handler.
 #[derive(Debug, Error)]
-pub enum GossipsubHandlerError {
+pub enum HandlerError {


This module is public (unfortunately), meaning this rename needs a deprecation type alias.

While we are at it, I think it would make sense to re-export all of these from the root and add deprecation type aliases that they shouldn't be imported from the error module so we can make it private in a future release.

I had to add type alias in error.rs too. Not sure if that's a neat solution here

protocols/gossipsub/src/lib.rs

protocols/gossipsub/src/behaviour.rs

thomaseizinger · 2023-01-11T04:13:41Z

protocols/gossipsub/src/config.rs

+    /// Calling `Builder::protocol_id_prefix` will set a new prefix and retain the prefix logic.
+    /// Calling `Builder::protocol_id` will set a custom `protocol_id` and disable the prefix logic.


Suggested change

/// Calling `Builder::protocol_id_prefix` will set a new prefix and retain the prefix logic.

/// Calling `Builder::protocol_id` will set a custom `protocol_id` and disable the prefix logic.

/// Calling [`Builder::protocol_id_prefix`] will set a new prefix and retain the prefix logic.

/// Calling [`Builder::protocol_id`] will set a custom `protocol_id` and disable the prefix logic.

Should be a proper type reference.

mergify · 2023-01-11T09:55:41Z

This pull request has merge conflicts. Could you please resolve them @StemCll? 🙏

StemCll · 2023-01-13T16:07:38Z

Got a question regarding force pushing - why isn't it suitable in this repo. Couldn't find anything in Contributing guide 🤔

Personally, I force push when conflicts occur - I'm rebasing trunk and then force pushing to remote. What do you prefer in these situations?

thomaseizinger · 2023-01-15T08:14:35Z

Got a question regarding force pushing - why isn't it suitable in this repo. Couldn't find anything in Contributing guide 🤔

We squash-merge all PRs so the commit history within a PR is not that important and thus regular merges are fine. They also play nicely with GitHub's review interface, allowing you to review only the new commits.

StemCll · 2023-01-15T19:34:07Z

I've pushed some changes, but will double-check all your comments. Will mention you when ready for re-review 😄

mergify · 2023-01-18T01:01:13Z

This pull request has merge conflicts. Could you please resolve them @StemCll? 🙏

StemCll · 2023-01-23T14:18:53Z

@thomaseizinger I've checked all your comments - right now the PR should be ready for review. Sorry that it took very long...

thomaseizinger

Well done, only a few more comments! :)

thomaseizinger · 2023-01-23T23:54:41Z

protocols/gossipsub/src/lib.rs

+#[deprecated(
+    since = "0.44.0",
+    note = "Use re-exports that omit `Gossipsub` prefix, i.e. `libp2p::gossipsub::HandlerError"
+)]
+pub type GossipsubHandlerError = HandlerError;


We previously didn't export this error on the root level right? I think this type alias is unnecessary.

It is slightly related but it would be really great if we can get rid of the public error module while we are at it. I'd prefer to not have any public modules actually. They are an easy semver hazard because types that shouldnt' be part of the public API easily slip into it.

To achieve this in a backwards-compatible way, we need to:

Make the error module private and rename it to error_priv

Make a new, inline error module that contains type aliases with deprecation notice, re-exporting the errors from error_priv

Export all errors in the root module (you are already doing this on line 171)

This allows us to migrate users towards importing the errors from the root module and we can remove the public error module in a future release!

See what we did in this PR for example: https://github.com/libp2p/rust-libp2p/pull/3238/files#diff-acd49d828b2b4f41554331034dd696f7946d8c1becc7635d118435454c262cde

Everything used to be under a v2 module that now only contains deprecated type-aliases that re-export symbols from their new location. Some of the new locations are crate-private however. In other words, we will be making certain types inaccessible in a future release. That is because they should have never been part of the public API.

I see some opportunities for this in gossipsub too:

protocol module

metrics (contains many public types and functions that a user doesn't need to interact with)

subscription_filter (can be completely flattened, i.e. re-export from root to shorten import paths)

time_cache (not sure why that is public at all?)

This is quite an undertaking by itself so it is probably best done in a separate PR. Let me know if you are up for it :)

Perfectly understandable. I like this direction. I'm not sure about making V2 APIs in these kind of changes, but let's talk about it in the next PR ;)

I've created a new issue to track these changes -> #3392

thomaseizinger · 2023-01-23T23:59:22Z

protocols/gossipsub/src/lib.rs

+
+pub use self::config::{Config, ConfigBuilder, ValidationMode, Version};
+


Suggested change

pub use self::config::{Config, ConfigBuilder, ValidationMode, Version};

pub use self::config::{Config, ConfigBuilder, ValidationMode, Version};

Let's not use more whitespace than necessary :)

thomaseizinger · 2023-01-23T23:59:31Z

protocols/gossipsub/src/lib.rs

@@ -156,18 +156,73 @@ mod types;

 mod rpc_proto;

-pub use self::behaviour::{Gossipsub, GossipsubEvent, MessageAuthenticity};
+pub use self::behaviour::{Behaviour, Event, MessageAuthenticity};
 pub use self::transform::{DataTransform, IdentityTransform};



Suggested change

thomaseizinger · 2023-01-24T00:10:57Z

@thomaseizinger I've checked all your comments - right now the PR should be ready for review. Sorry that it took very long...

No worries at all, thank you for pushing this forward!

mergify · 2023-01-26T14:41:52Z

This pull request has merge conflicts. Could you please resolve them @StemCll? 🙏

… lib

thomaseizinger

Thanks! This looks good. I double checked our exports again, they all match the old types :)

protocols/gossipsub/CHANGELOG.md

Approvals have been dismissed because the PR was updated after the send-it label was applied.

protocols/gossipsub/CHANGELOG.md

Approvals have been dismissed because the PR was updated after the send-it label was applied.

StemCll force-pushed the chore/rename_gossipsub_symbols branch 2 times, most recently from 268e519 to e2e3e51 Compare January 4, 2023 15:24

StemCll changed the title ~~WIP: refactor(gossipsub): gossipsub naming revision~~ Refactor(gossipsub): gossipsub naming revision Jan 5, 2023

StemCll changed the title ~~Refactor(gossipsub): gossipsub naming revision~~ refactor(gossipsub): gossipsub naming revision Jan 5, 2023

StemCll force-pushed the chore/rename_gossipsub_symbols branch from 909b206 to b13b0e1 Compare January 5, 2023 12:04

StemCll commented Jan 5, 2023

View reviewed changes

examples/gossipsub-chat.rs Outdated Show resolved Hide resolved

StemCll commented Jan 5, 2023

View reviewed changes

protocols/gossipsub/src/lib.rs Outdated Show resolved Hide resolved

thomaseizinger reviewed Jan 9, 2023

View reviewed changes

StemCll force-pushed the chore/rename_gossipsub_symbols branch from b9c3218 to d5d2a2e Compare January 10, 2023 12:45

thomaseizinger mentioned this pull request Jan 11, 2023

refactor(gossipsub): remove unnecessary trait bounds #3317

Merged

4 tasks

thomaseizinger requested changes Jan 11, 2023

View reviewed changes

StemCll force-pushed the chore/rename_gossipsub_symbols branch from cba9823 to 51e2bf6 Compare January 13, 2023 16:02

StemCll force-pushed the chore/rename_gossipsub_symbols branch from 2143e09 to 6e6445d Compare January 23, 2023 14:16

StemCll requested a review from thomaseizinger January 23, 2023 14:16

thomaseizinger changed the title ~~refactor(gossipsub): gossipsub naming revision~~ refactor(gossipsub): revise symbol naming to follow conventions Jan 23, 2023

thomaseizinger reviewed Jan 24, 2023

View reviewed changes

thomaseizinger mentioned this pull request Jan 24, 2023

refactor(gossipsub)!: initialize ProtocolConfig from GossipsubConfig #3381

Merged

4 tasks

StemCll added 3 commits January 26, 2023 19:20

Rename types in gossipsub

0303849

Examples fix

11824b7

Rename GossipsubEvent to Event

6b8ba63

StemCll added 11 commits January 26, 2023 19:33

Fix examples and gossipsub/tests/smoke

bea120f

Rename GossipsubVersion to Version

be1503b

Add deprecations in lib.rs; make config module public

d42a073

Fix doc's code

d328eaa

Rename ProstMessage to ProtobufMessage in gossipsub/types

a061f87

Revert Event::ProtobufMessage to Event:Message

b6976d7

Remove config prefix from Config, Builder and Version

25fe3ce

Revert renaming from Gossipsub to Behaviour in comments

1a4d6c6

Rename config::builder to ConfigBuilder; fix & add missing exports in…

5f5d2c6

… lib

Add documentation renamings

2717c14

Fix rebase naming issues; remove whitespaces in lib.rs

d9efbc3

StemCll force-pushed the chore/rename_gossipsub_symbols branch from 6e6445d to d9efbc3 Compare January 26, 2023 18:53

StemCll added 2 commits January 26, 2023 20:06

Add CHANGELOG entry

07437bb

Remove redundant deprecation

eff093a

StemCll mentioned this pull request Jan 26, 2023

gossipsub: make modules private #3392

Closed

thomaseizinger previously approved these changes Jan 26, 2023

View reviewed changes

protocols/gossipsub/CHANGELOG.md Outdated Show resolved Hide resolved

Update protocols/gossipsub/CHANGELOG.md

8d661ea

thomaseizinger added the send-it label Jan 26, 2023

thomaseizinger reviewed Jan 27, 2023

View reviewed changes

protocols/gossipsub/CHANGELOG.md Outdated Show resolved Hide resolved

Update protocols/gossipsub/CHANGELOG.md

4680e87

thomaseizinger previously approved these changes Jan 27, 2023

View reviewed changes

thomaseizinger approved these changes Jan 27, 2023

View reviewed changes

Merge branch 'master' into chore/rename_gossipsub_symbols

2d983c1

mergify bot merged commit ab59af4 into libp2p:master Jan 27, 2023

jxs mentioned this pull request Jan 27, 2023

All crates should have a consistent API & naming #2217

Open

StemCll mentioned this pull request Feb 12, 2023

refactor(gossipsub): make error module private #3457

Merged

4 tasks

StemCll deleted the chore/rename_gossipsub_symbols branch February 22, 2023 17:51

StemCll mentioned this pull request Feb 22, 2023

refactor gossipsub: make modules private #3494

Closed

4 tasks

	use prost::Message as ProtobufMessage;
	use prost::Message as _;

		/// Calling `Builder::protocol_id_prefix` will set a new prefix and retain the prefix logic.
		/// Calling `Builder::protocol_id` will set a custom `protocol_id` and disable the prefix logic.


		pub use self::config::{Config, ConfigBuilder, ValidationMode, Version};

refactor(gossipsub): revise symbol naming to follow conventions #3303

refactor(gossipsub): revise symbol naming to follow conventions #3303

Conversation

StemCll commented Jan 2, 2023 • edited Loading

Description

Notes

Links to any relevant issues

Open Questions

Change checklist

mergify bot commented Jan 3, 2023

StemCll commented Jan 5, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Jan 11, 2023

StemCll commented Jan 13, 2023

thomaseizinger commented Jan 15, 2023

StemCll commented Jan 15, 2023

mergify bot commented Jan 18, 2023

StemCll commented Jan 23, 2023 • edited Loading

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger commented Jan 24, 2023

mergify bot commented Jan 26, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

StemCll commented Jan 2, 2023 •

edited

Loading

StemCll commented Jan 23, 2023 •

edited

Loading