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

feat: apply nursery lints #202

Merged
merged 15 commits into from
May 1, 2024
Merged

feat: apply nursery lints #202

merged 15 commits into from
May 1, 2024

Conversation

CosminPerRam
Copy link
Contributor

@CosminPerRam CosminPerRam commented Apr 29, 2024

This PR upgrades clippy to warn about nursery lints on CI and applies said lints.

Applied changes:

  • Make applicable functions constants
  • Replace usage of pub(crate) with pub in already private modules.
  • Replace unnecessary struct name repetitions (e.g.: when implementing From for an enum, inside said functions you dont have to use Enum::Variant as it's already implying its usage, therefore using Self instead of Enum).
  • Derive Eq for DnsEntry, it already implements PartialEq but Eq is also applicable.
  • Replace some ifs/matches with dedicated methods (an example of this is Option::map_or_else which is more cleaner and concise).
  • Add allow(clippy::cognitive_complexity) for the service_daemon module, the lint rates the complexity of a function and warns if it is exceeded, in this case 2 functions pass the default threshold, the solution to this is to make it smaller by refactoring, splitting it into multiple functions etc (or maybe not possible in some cases), I've added this as solving it would be outside of the PR's scope, so lets make CI pass.

This is a 'low-hanging fruit' PR that attempts on making the code a bit more concise (and possible more performant because of the constants and/or dedicated methods for if/match).

Note: the nursery lint group are "new lints that are still under development", if you might not want it, I'll remove it from the CI file.

Copy link
Owner

@keepsimple1 keepsimple1 left a comment

Choose a reason for hiding this comment

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

Thanks for your PR! Some neat changes are new knowledge to me ;-) .

Please see inline comments. Overall looks good, and I think it's cool to run nursery lints once a while locally and adopt some suggestions, even though I don't think we're ready to codify it in CI yet.

@@ -27,6 +27,6 @@ jobs:
- name: Build
run: cargo build
- name: Run clippy and fail if any warnings
run: cargo clippy -- -D warnings
run: cargo clippy -- -W clippy::all -W clippy::nursery -D warnings
Copy link
Owner

Choose a reason for hiding this comment

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

It's ok to specify -W clippy::all , but I don't think we need to have clippy::nursery in CI yet.

src/lib.rs Outdated
@@ -147,6 +147,7 @@ mod log {

mod dns_parser;
mod error;
#[allow(clippy::cognitive_complexity)]
Copy link
Owner

Choose a reason for hiding this comment

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

Since I suggest we don't add clippy::nursery in CI, we don't need this line either.

@keepsimple1
Copy link
Owner

  • Add allow(clippy::cognitive_complexity) for the service_daemon module, the lint rates the complexity of a function and warns if it is exceeded, in this case 2 functions pass the default threshold, the solution to this is to make it smaller by refactoring, splitting it into multiple functions etc (or maybe not possible in some cases), I've added this as solving it would be outside of the PR's scope, so lets make CI pass.

It's cool to learn about this (for me). Although I opted not to mandate cognitive_complexity lint, I'm curious what the two functions are. (A quick local run didn't turn up such warnings unless I missed them).

@CosminPerRam
Copy link
Contributor Author

  • Add allow(clippy::cognitive_complexity) for the service_daemon module, the lint rates the complexity of a function and warns if it is exceeded, in this case 2 functions pass the default threshold, the solution to this is to make it smaller by refactoring, splitting it into multiple functions etc (or maybe not possible in some cases), I've added this as solving it would be outside of the PR's scope, so lets make CI pass.

It's cool to learn about this (for me). Although I opted not to mandate cognitive_complexity lint, I'm curious what the two functions are. (A quick local run didn't turn up such warnings unless I missed them).

Yeah, very interesting to see that clippy nursery suggests splitting big chunks of code/complexity.

    Checking mdns-sd v0.11.0 (/Users/cosminpatrinjan/Github/_contributing/mdns-sd)
error: the function has a cognitive complexity of (50/25)
   --> src/service_daemon.rs:624:8
    |
624 |     fn exec_command(zc: &mut Zeroconf, command: Command, repeating: bool) {
    |        ^^^^^^^^^^^^
    |
    = help: you could split it up into multiple smaller functions
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#cognitive_complexity
    = note: `-D clippy::cognitive-complexity` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::cognitive_complexity)]`

error: the function has a cognitive complexity of (26/25)
    --> src/service_daemon.rs:1970:8
     |
1970 |     fn handle_query(&mut self, msg: DnsIncoming, ip: &IpAddr) {
     |        ^^^^^^^^^^^^
     |
     = help: you could split it up into multiple smaller functions
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#cognitive_complexity

error: could not compile `mdns-sd` (lib) due to 2 previous errors

@CosminPerRam CosminPerRam changed the title clippy: upgrade to nursery and apply lints feat: apply nursery lints Apr 30, 2024
@keepsimple1
Copy link
Owner

Yeah, very interesting to see that clippy nursery suggests splitting big chunks of code/complexity.

    Checking mdns-sd v0.11.0 (/Users/cosminpatrinjan/Github/_contributing/mdns-sd)
error: the function has a cognitive complexity of (50/25)
   --> src/service_daemon.rs:624:8
    |
624 |     fn exec_command(zc: &mut Zeroconf, command: Command, repeating: bool) {
    |        ^^^^^^^^^^^^
    |
    = help: you could split it up into multiple smaller functions
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#cognitive_complexity
    = note: `-D clippy::cognitive-complexity` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::cognitive_complexity)]`

error: the function has a cognitive complexity of (26/25)
    --> src/service_daemon.rs:1970:8
     |
1970 |     fn handle_query(&mut self, msg: DnsIncoming, ip: &IpAddr) {
     |        ^^^^^^^^^^^^
     |
     = help: you could split it up into multiple smaller functions
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#cognitive_complexity

error: could not compile `mdns-sd` (lib) due to 2 previous errors

cool! we can look into them. I opened a separate issue #204 to track this .

Copy link
Owner

@keepsimple1 keepsimple1 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@keepsimple1 keepsimple1 merged commit 5732665 into keepsimple1:main May 1, 2024
3 checks passed
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.

None yet

2 participants