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

Do not suggest replacing simple if let Some(_) / None with methods #2353

Closed
bluss opened this issue Jan 14, 2018 · 5 comments
Closed

Do not suggest replacing simple if let Some(_) / None with methods #2353

bluss opened this issue Jan 14, 2018 · 5 comments
Labels
S-needs-discussion Status: Needs further discussion before merging or work can be started

Comments

@bluss
Copy link
Member

bluss commented Jan 14, 2018

This lint I don't quite agree with, and I realize opinions will differ on this.

Lint link: https://rust-lang-nursery.github.io/rust-clippy/v0.0.177/index.html#if_let_redundant_pattern_matching

Existing code in removed (red) and the proposed change by clippy in added (green) lines:

         // clear edges without touching the free list
         for node in &mut self.g.nodes {
-            if let Some(_) = node.weight {
+            if node.weight.is_some() {
                 node.next = [EdgeIndex::end(), EdgeIndex::end()];
             }
         }
-        if let None = node_weight {
+        if node_weight.is_none() {
             return None;
         }

etc with similar examples.

I think that using if let is fundamental to Rust, is general, works with any enum and is just as easy to understand as the alternative. I prefer general over Option-specific methods.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 15, 2018

Advantages of Option methods:

  • can chain with other bool operations
    • thus trigger more clippy lints down the road
  • fewer characters
  • left to right reading order

Disadvantages of Option methods

  • methods calls can be anything
  • method calls are not fundamental to Rust (😆)
  • more code for the optimizer to handle

I don't have a strong opinion either way. I just do what clippy says

We could make it a restriction lint, but then I'd also like to see the opposite lint.


Side note:

-        if let None = node_weight {
+        if node_weight.is_none() {
             return None;
         }

shouldn't that simply be node_weight?;

@oli-obk oli-obk added the S-needs-discussion Status: Needs further discussion before merging or work can be started label Jan 15, 2018
@shssoichiro
Copy link
Contributor

shouldn't that simply be node_weight?;

I'd like to see a lint for that. :)

@ghost
Copy link

ghost commented Jan 16, 2018

Please suggest replacing o.is_some() with o.iter().count() == 1 and o.is_none() with o.iter().count() == 0.

I think that using collections is fundamental to Rust. This method is general, works with any collection and is just as easy to understand as the alternative. I prefer general over Option-specific methods.

@Manishearth
Copy link
Member

No trolling, please.

@camsteffen
Copy link
Contributor

Given that this hasn't received any "me too"s, I think people are generally satisfied with the current behavior. To me is_some easily carries its weight because Option is just so ubiquitous to Rust.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-needs-discussion Status: Needs further discussion before merging or work can be started
Projects
None yet
Development

No branches or pull requests

5 participants