add count_threshold option #142

devvmh · 2016-10-18T07:15:08Z

adds the option to only check duplication if the code is found in more than just two locations.

fixes #131

devvmh · 2016-10-18T07:17:04Z

lib/cc/engine/analyzers/reporter.rb

@@ -51,6 +51,7 @@ def report
            violations = new_violations(issue)

            violations.each do |violation|
+              next if (violation.occurrences + 1) < language_strategy.count_threshold


here's where the magic happens

dblandin

This PR looks great to me! One minor style suggestion and I'm also wondering if there's a spec we can add that exercises the change in the Reporter class.

Thanks!

dblandin · 2016-10-20T18:08:28Z

lib/cc/engine/analyzers/engine_config.rb

@@ -30,6 +30,11 @@ def mass_threshold_for(language)
          end
        end

+        def count_threshold_for(language)
+          threshold = fetch_language(language).fetch("count_threshold", nil)
+          threshold ? threshold.to_i : threshold


This ternary conditional is a bit difficult to read when the switch value is also the default return.

Given the similarity to the #mass_threshold_for method above, it might make sense to consolidate:

threshold_config_for(:mass, "ruby") threshold_config_for(:count, "ruby") def threshold_config_for(option, language) threshold = fetch_language(language)["#{option}_threshold"] if threshold threshold.to_i end end

I think (and my feelings on this are related to the origins of this PR), that there's no point in abstracting these two methods until there's 3-4 similar config options (and I really do think there won't be any more)

You could also even do

def count_threshold_for(language) fetch_language(language).dig("count_threshold")&.to_i end

if we wanted to get really crazy (well, I guess in ruby 2.3 only)

I'll switch it back to the old if threshold syntax

Good point! 👍

devvmh · 2016-10-21T08:45:39Z

I tried to update the spec, but it looks pretty involved because of where I put the code. Maybe I'll try again next week if I have time

dblandin · 2016-10-24T19:13:02Z

@devvmh I'm good rolling with this as is. Going to do some local testing off of this branch before merging.

devvmh · 2016-10-25T02:47:37Z

Great, thanks!

dblandin · 2016-10-27T16:20:28Z

Hey @devvmh,

I've tested this locally and it works well! One question I have now that I've thought about it a bit more is whether this option should be configured one level up instead of nested under a particular language.

Do you think it's likely that users will want to specify different count thresholds per language? The mass_threshold configuration option works that way because of the difference in sensitivity between different language parser ASTs.

I'm thinking that it might be better to support this option as a sibling to languages:

engines:
  duplication:
    enabled: true
    config:
      count_threshold: 3
      languages:
        - ruby

What do you think?

devvmh · 2016-10-28T01:37:54Z

Yeah that's a good question. I think for my particular case I would do the same for ruby and javascript. I do think that it's more useful to be per-language than not though, don't you? Like, it's not harder to configure per-language (just 4 extra lines of YAML worst case).

If you'd like me to change it, just say and I can do that

dblandin · 2016-10-28T17:03:14Z

Just took a quick poll internally, and the consensus was basically:

So I think it would be ideal if we supported the configuration option at the higher position, but also allow language-specific overrides.

engines:
  duplication:
    enabled: true
    config:
      count_threshold: 3
      languages:
        ruby:
        python:
        javascript:
          count_threshold: 5

Does that sound good to you?

devvmh · 2016-10-29T01:18:44Z

Yeah! brilliant. I'll tty and implement that in tte jext few days

On Sat, Oct 29, 2016, 1:03 AM Devon Blandin notifications@github.com
wrote:

Just took a quick poll internally, and the consensus was basically:

[image: giphy]
https://cloud.githubusercontent.com/assets/123595/19814977/a0582e40-9d0e-11e6-8d2e-c5c7e6723c8c.gif

So I think it would be ideal if we supported the configuration level at
the higher position, but also allowed language-specific overrides.

engines:
duplication:
enabled: true
config:
count_threshold: 3
languages:
    ruby:
    python:
    javascript:
      count_threshold: 5
Does that sound good to you?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#142 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABVELFIPvdTKm5fTTe9zMgb1S2eTe9rOks5q4irSgaJpZM4KZcxt
.

dblandin · 2016-10-31T21:30:13Z

lib/cc/engine/analyzers/engine_config.rb

@@ -30,6 +30,15 @@ def mass_threshold_for(language)
          end
        end

+        def count_threshold_for(language)
+          threshold = fetch_language(language).fetch("count_threshold", nil)


What do you think about pushing the default count_threshold value into the EngineConfig class since languages don't have specific defaults?

module CC module Engine module Analyzers class EngineConfig DEFAULT_COUNT_THRESHOLD = 2 def count_threshold_for(language) fetch_language(language)["count_threshold"] || count_threshold || DEFAULT_COUNT_THRESHOLD end private def count_threshold config["count_threshold"] end

module CC module Engine module Analyzers class Base def count_threshold engine_config.count_threshold_for(self.class::LANGUAGE) end

devvmh · 2016-11-05T21:28:43Z

Phew, OK, that's finally updated.

dblandin · 2016-11-05T22:23:20Z

@devvmh Awesome! The code looks great 👍

Last thing: Would you mind adding an example or two to spec/cc/engine/analyzers/engine_config_spec.rb to cover EngineConfig#count_threshold_for?

adds the option to only check duplication if the code is found in more than just two locations.

devvmh · 2016-11-07T18:43:19Z

OK, updated. Should I squash this into one commit?

dblandin · 2016-11-07T19:31:05Z

@devvmh Perfect 👌

Should I squash this into one commit?

No need. I'm going use GitHub's handy "Squash and merge" button.

Thanks for the contribution! 🎉

devvmh force-pushed the feature/count-threshold branch from fc38551 to b39d39f Compare October 18, 2016 07:16

devvmh commented Oct 18, 2016

View reviewed changes

devvmh mentioned this pull request Oct 18, 2016

Only report if there are 3 occurrences of the code? #131

Closed

dblandin reviewed Oct 20, 2016

View reviewed changes

dblandin reviewed Oct 31, 2016

View reviewed changes

devvmh force-pushed the feature/count-threshold branch from 5289dbb to 5adf9d3 Compare November 7, 2016 18:42

devvmh added 5 commits November 7, 2016 13:42

add count_threshold option

7768635

adds the option to only check duplication if the code is found in more than just two locations.

make count_threshold_for less confusing

6418c2d

allow changing global default count threshold

8ff3143

make count threshold default no longer language-specific

b5c1a69

spec for count_threshold_for

96cd4ef

devvmh force-pushed the feature/count-threshold branch from 5adf9d3 to 96cd4ef Compare November 7, 2016 18:43

dblandin merged commit df62039 into codeclimate:master Nov 7, 2016

devvmh deleted the feature/count-threshold branch November 7, 2016 19:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add count_threshold option #142

add count_threshold option #142

devvmh commented Oct 18, 2016

devvmh Oct 18, 2016

dblandin left a comment

dblandin Oct 20, 2016

devvmh Oct 21, 2016 •

edited

Loading

dblandin Oct 24, 2016

devvmh commented Oct 21, 2016

dblandin commented Oct 24, 2016

devvmh commented Oct 25, 2016

dblandin commented Oct 27, 2016

devvmh commented Oct 28, 2016

dblandin commented Oct 28, 2016 •

edited

Loading

devvmh commented Oct 29, 2016

dblandin Oct 31, 2016

devvmh commented Nov 5, 2016

dblandin commented Nov 5, 2016

devvmh commented Nov 7, 2016

dblandin commented Nov 7, 2016

add count_threshold option #142

add count_threshold option #142

Conversation

devvmh commented Oct 18, 2016

devvmh Oct 18, 2016

Choose a reason for hiding this comment

dblandin left a comment

Choose a reason for hiding this comment

dblandin Oct 20, 2016

Choose a reason for hiding this comment

devvmh Oct 21, 2016 • edited Loading

Choose a reason for hiding this comment

dblandin Oct 24, 2016

Choose a reason for hiding this comment

devvmh commented Oct 21, 2016

dblandin commented Oct 24, 2016

devvmh commented Oct 25, 2016

dblandin commented Oct 27, 2016

devvmh commented Oct 28, 2016

dblandin commented Oct 28, 2016 • edited Loading

devvmh commented Oct 29, 2016

dblandin Oct 31, 2016

Choose a reason for hiding this comment

devvmh commented Nov 5, 2016

dblandin commented Nov 5, 2016

devvmh commented Nov 7, 2016

dblandin commented Nov 7, 2016

devvmh Oct 21, 2016 •

edited

Loading

dblandin commented Oct 28, 2016 •

edited

Loading