{"action":"created","issue":{"url":"https://api.github.com/repos/Manishearth/rust-clippy/issues/789","repository_url":"https://api.github.com/repos/Manishearth/rust-clippy","labels_url":"https://api.github.com/repos/Manishearth/rust-clippy/issues/789/labels{/name}","comments_url":"https://api.github.com/repos/Manishearth/rust-clippy/issues/789/comments","events_url":"https://api.github.com/repos/Manishearth/rust-clippy/issues/789/events","html_url":"https://github.com/Manishearth/rust-clippy/issues/789","id":142787894,"number":789,"title":"`collapsible_if` should have some sort of maximum complexity","user":{"login":"Nemo157","id":81079,"avatar_url":"https://avatars.githubusercontent.com/u/81079?v=3","gravatar_id":"","url":"https://api.github.com/users/Nemo157","html_url":"https://github.com/Nemo157","followers_url":"https://api.github.com/users/Nemo157/followers","following_url":"https://api.github.com/users/Nemo157/following{/other_user}","gists_url":"https://api.github.com/users/Nemo157/gists{/gist_id}","starred_url":"https://api.github.com/users/Nemo157/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Nemo157/subscriptions","organizations_url":"https://api.github.com/users/Nemo157/orgs","repos_url":"https://api.github.com/users/Nemo157/repos","events_url":"https://api.github.com/users/Nemo157/events{/privacy}","received_events_url":"https://api.github.com/users/Nemo157/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2016-03-22T21:35:59Z","updated_at":"2016-03-22T21:46:36Z","closed_at":null,"body":"Not sure if cyclomatic complexity of the checks or just purely span size should be taken into account, but with the following snippet of code (taken from [here](https://git.nemo157.com/grarr/blob/05dc0d51fa6659ee5d01cf318524909a6ebc0df8/src/handler/git_smart_http/upload_pack.rs#L124)) I get `collapsible_if` warnings that don't really make sense to implement\r\n\r\n```rust\r\n// if it is an ancestor of a ref\r\nif try!(graph_ancestor_of_any(&context.repository, id, context.refs.iter().cloned())) {\r\n  // and is not an ancestor of a common\r\n  if !try!(graph_ancestor_of_any(&context.repository, id, common.iter().cloned())) {\r\n    // add it to common\r\n    common.insert(id);\r\n  }\r\n}\r\n```\r\n\r\nwarning message:\r\n\r\n```\r\nsrc/handler/git_smart_http/upload_pack.rs:124:5: 130:6 warning: this if statement can be collapsed, #[warn(collapsible_if)] on by default\r\nsrc/handler/git_smart_http/upload_pack.rs:124     if try!(graph_ancestor_of_any(&context.repository, id, context.refs.iter().cloned())) {\r\nsrc/handler/git_smart_http/upload_pack.rs:125       // and is not an ancestor of a common\r\nsrc/handler/git_smart_http/upload_pack.rs:126       if !try!(graph_ancestor_of_any(&context.repository, id, common.iter().cloned())) {\r\nsrc/handler/git_smart_http/upload_pack.rs:127         // add it to common\r\nsrc/handler/git_smart_http/upload_pack.rs:128         common.insert(id);\r\nsrc/handler/git_smart_http/upload_pack.rs:129       }\r\n                                              ...\r\nsrc/handler/git_smart_http/upload_pack.rs:124:5: 130:6 help: try\r\nsrc/handler/git_smart_http/upload_pack.rs:        if (try!(graph_ancestor_of_any(&context.repository, id, context.refs.iter().cloned()))) && (!try!(graph_ancestor_of_any(&context.repository, id, common.iter().cloned()))) {\r\nsrc/handler/git_smart_http/upload_pack.rs:      // add it to common\r\nsrc/handler/git_smart_http/upload_pack.rs:      common.insert(id);\r\nsrc/handler/git_smart_http/upload_pack.rs:    }\r\nsrc/handler/git_smart_http/upload_pack.rs:124:5: 130:6 help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#collapsible_if\r\n```\r\n\r\nIf I were to follow the suggestion it would result in:\r\n\r\n```rust\r\n// if it is an ancestor of a ref and is not an ancestor of a common\r\nif try!(graph_ancestor_of_any(&context.repository, id, context.refs.iter().cloned())) && !try!(graph_ancestor_of_any(&context.repository, id, common.iter().cloned())) {\r\n  // add it to common\r\n  common.insert(id);\r\n}\r\n```\r\n\r\nwhich is obviously far too long to actually use (and line breaks in conditional checks are horrible). The only \"correct\" solution to this warning that I can think of without pulling these tests out to helper functions would be something like:\r\n\r\n```rust\r\nlet refs_iter = context.refs.iter().cloned();\r\nlet common_iter = common.iter().cloned();\r\nlet ancestor_of_ref = try!(graph_ancestor_of_any(&context.repository, id, refs_iter));\r\nlet ancestor_of_common = || graph_ancestor_of_any(&context.repository, id, common_iter);\r\n// if it is an ancestor of a ref and is not an ancestor of a common\r\nif ancestor_of_ref && !try!(ancestor_of_common()) {\r\n  // add it to common\r\n  common.insert(id);\r\n}\r\n```\r\n\r\nwhich results in needing to pull the iter creation out to stay under 100 characters and using a closure to retain the short-circuiting nature of `&&` with these relatively expensive tests."},"comment":{"url":"https://api.github.com/repos/Manishearth/rust-clippy/issues/comments/200044410","html_url":"https://github.com/Manishearth/rust-clippy/issues/789#issuecomment-200044410","issue_url":"https://api.github.com/repos/Manishearth/rust-clippy/issues/789","id":200044410,"user":{"login":"mcarton","id":3751788,"avatar_url":"https://avatars.githubusercontent.com/u/3751788?v=3","gravatar_id":"","url":"https://api.github.com/users/mcarton","html_url":"https://github.com/mcarton","followers_url":"https://api.github.com/users/mcarton/followers","following_url":"https://api.github.com/users/mcarton/following{/other_user}","gists_url":"https://api.github.com/users/mcarton/gists{/gist_id}","starred_url":"https://api.github.com/users/mcarton/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mcarton/subscriptions","organizations_url":"https://api.github.com/users/mcarton/orgs","repos_url":"https://api.github.com/users/mcarton/repos","events_url":"https://api.github.com/users/mcarton/events{/privacy}","received_events_url":"https://api.github.com/users/mcarton/received_events","type":"User","site_admin":false},"created_at":"2016-03-22T21:46:36Z","updated_at":"2016-03-22T21:46:36Z","body":"Clippy does try to format the suggestion. You can just split the the condition on two lines.\r\nOr alternatively\r\n```rust\r\nlet is_ancestor_of = |e| graph_ancestor_of_any(&context.repository, id, e.iter().cloned());\r\nif try!(is_ancestor_of(context.refs)) && try!(is_ancestor_of(common)) {\r\n    common.insert(id);\r\n}\r\n```\r\nOr just `#[allow]` the lint.\r\nI’m not sure the additional complexity is worth it. Especially since most lints that suggest new code might be affected by bad formatting."}}