Skip to content

Commit

Permalink
Categorize various flavors of if in Ruby as if nodes (#1361)
Browse files Browse the repository at this point in the history
In https://github.com/orgs/qltysh/discussions/1313 @nk-ty noted that
`qlty` and `codeclimate` CLIs calculate the cognitive complexity of a
method with trailing unless statements differently. After some
investigation, I determined that both were incorrectly calculating
cognitive complexity. This PR is concerned with the cognitive complexity
calculation in qlty.

The following are equivalent ways of expressing an `if` conditional in
Ruby:

```ruby
if !foo
    puts "hi"
end
```

```ruby
unless foo
    puts "foo"
end
```

```ruby
puts "hi" if !foo
```

```ruby
puts "hi" unless foo
```

The first was correctly being calculated. The second was an obvious
oversight. And the 3rd and 4th ones were less obvious oversights (the
parser calls them `if_modifier` / `unless_modifier` nodes not `if` /
`unless` nodes.

This PR categorizes all 4 as `if_nodes` which the cognitive complexity
calculator uses, and correctly penalizes all 4 as 1 point.
  • Loading branch information
noahd1 authored Dec 23, 2024
1 parent 0814df2 commit 7ac9ca2
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 1 deletion.
10 changes: 9 additions & 1 deletion qlty-analysis/src/lang/ruby.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,9 @@ impl Ruby {
pub const GLOBAL_VARIABLE: &'static str = "global_variable";
pub const IDENTIFIER: &'static str = "identifier";
pub const IF: &'static str = "if";
pub const UNLESS: &'static str = "unless";
pub const IF_MODIFIER: &'static str = "if_modifier"; // statement _modifier_ aka trailing if
pub const UNLESS_MODIFIER: &'static str = "unless_modifier"; // statement _modifier_ aka trailing unless
pub const INITIALIZE: &'static str = "initialize";
pub const INSTANCE_VARIABLE: &'static str = "instance_variable";
pub const METHOD_CALL: &'static str = "method_call";
Expand Down Expand Up @@ -146,7 +149,12 @@ impl Language for Ruby {
}

fn if_nodes(&self) -> Vec<&str> {
vec![Self::IF]
vec![
Self::IF,
Self::UNLESS,
Self::IF_MODIFIER,
Self::UNLESS_MODIFIER,
]
}

fn elsif_nodes(&self) -> Vec<&str> {
Expand Down
27 changes: 27 additions & 0 deletions qlty-smells/src/metrics/metrics/cognitive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,33 @@ mod test {
);
}

#[test]
fn count_statement_modifiers() {
let source_file = File::from_string(
"ruby",
r#"
def foo(bar)
return 1 unless bar # +1
return 2 if bar # +1
if bar # +1
return 3
end
unless bar # +1
return 4
end
end
"#,
);
assert_eq!(
4,
count(
&source_file,
&source_file.parse().root_node(),
&NodeFilter::empty()
)
);
}

#[test]
fn count_else_if() {
let source_file = File::from_string(
Expand Down

0 comments on commit 7ac9ca2

Please sign in to comment.