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

*: Activate clippy::style lint group #2620

Merged
merged 8 commits into from
May 3, 2022

Conversation

LesnyRumcajs
Copy link
Contributor

@LesnyRumcajs LesnyRumcajs commented Apr 25, 2022

Description

Enabled clippy style warnings and fixed the corresponding issues. I had to suppress some of them to not break the existing API (we could consider resolving those for a major release).

Links to any relevant issues

#2615

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

@LesnyRumcajs LesnyRumcajs changed the title Fix clippy style Enable and fix clippy style issues Apr 25, 2022
@LesnyRumcajs LesnyRumcajs force-pushed the fix-clippy-style branch 2 times, most recently from d8f1d11 to 44d7034 Compare April 25, 2022 18:44
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.

I am a fan of clippy myself, happy to move forward with this.
What do you think @mxinden?

protocols/rendezvous/src/codec.rs Outdated Show resolved Hide resolved
swarm/src/connection.rs Outdated Show resolved Hide resolved
Comment on lines 1229 to 1235
if !self.duplicate_cache.contains(&id)
&& !self.pending_iwant_msgs.contains(&id)
&& self
.peer_score
.as_ref()
.map(|(_, _, _, promises)| !promises.contains(&id))
.unwrap_or(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is quite the monster boolean condition now. Perhaps we should extract 3 boolean variables that we can then chain together to make this more readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For readability, it would make sense, but it would kill the possibility of short-circuiting, which may result in a performance drop.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess functions would allow for readability and short-circuiting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, a lambda would do the trick! For the first two conditions I don't think it would help much, i.e. !self.duplicate_cache.contains(&id) would most likely become duplicate_cache.contains(&id). Is it fine for you?

The last one is definitely a monster though and I'd be happy to turn it into a short lambda. Any suggestions on a meaningful name for it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably even better would be to do:

impl Gossipsub {
	fn want_message(&self, id: ???) -> bool {
		if self.duplicate_cache.contains(&id) {
			return false;
		}

		if self.pending_iwant_msgs.contains(&id) {
			return false;
		}

		self.peer_score
     		  .as_ref()
            .map(|(_, _, _, promises)| !promises.contains(&id))
            .unwrap_or(true)
	}
}

Although I think clippy might complain about this one being possible to write as a && chain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I went ahead with similar change. I was a bit confused with a contains method being mutable for no visible reason so I made them immutable. Maybe pedantic clippy would have caught that before.

@dignifiedquire
Copy link
Member

big fan of clippy myself, very much in favor of this in general

@thomaseizinger
Copy link
Contributor

Side-note @LesnyRumcajs : We use squash-merge so please don't force-push :)
Makes it easier to see the diff since the last review!

@thomaseizinger thomaseizinger changed the title Enable and fix clippy style issues *: Activate clippy::style lint group Apr 27, 2022
@LesnyRumcajs
Copy link
Contributor Author

Side-note @LesnyRumcajs : We use squash-merge so please don't force-push :) Makes it easier to see the diff since the last review!

Sure thing. I was uncertain how it would behave coming from my fork which was a bit outdated so I defaulted to my typical routine. :)

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.

Just one nit pick. Otherwise looks good to me. Thanks for the help!

protocols/kad/CHANGELOG.md Outdated Show resolved Hide resolved
@mxinden mxinden merged commit 70d3852 into libp2p:master May 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants