Skip to content

Tune remediation points #42

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 29, 2015
Merged

Tune remediation points #42

merged 2 commits into from
Dec 29, 2015

Conversation

ABaldwinHunter
Copy link
Contributor

Tune complexity points, following calculation function in codeclimate-eslint, creating more parity with Ruby analysis in Code Climate classic.

cc @codeclimate/review @noahd1

ABaldwinHunter
ABaldwinHunter reviewed Dec 22, 2015
View reviewed changes
spec/cc/engine/rubocop_spec.rb
@@ -323,7 +323,7 @@ def includes_check?(output, cop_name)
def includes_content_for?(output, cop_name)
issue = issues(output).detect { |i| i["check_name"] =~ /#{cop_name}$/ }

issue["content"]["body"].present?
issue["content"] && issue["content"]["body"].present?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change isn't related, but came up when running tests. Happy to remove for cleaner diff if preferred.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with this drive-by, but I'd make it its own commit if you can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. Moved to separate commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still only see one commit on this branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhh sorry.
Fixed. Then made additional changed and squashed everything again. My bad. Updating.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Thanks for the catch.

@ABaldwinHunter ABaldwinHunter force-pushed the abh-tune-remediation-points branch from 638193e to 70a6461 Compare December 22, 2015 17:12
mrb
mrb reviewed Dec 22, 2015
View reviewed changes
config/cops.yml
Metrics/CyclomaticComplexity:
base_points: 75_000
violation_points: 10_000
base_points: 1_000_000
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this effectively mean that any instance of Cyclomatic complexity makes a file a "B"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2,000,000 remediation points are needed in penalties to go from A to B.

So not always, but a lot of the time an instance of Cyclomatic Complexity could have that large of grade impact.

I was talking with @brynary yesterday and had the impression that that level of grade impact would be close to what we want for complexity - but maybe requires some additional discussion?

@ABaldwinHunter
Copy link
Contributor Author

@codeclimate/review @brynary

Ready for another look!

@ABaldwinHunter
Copy link
Contributor Author

from commit message:

In Classic Analysis, typical remediation point formula for complexity were
roughly:

Class
  5_000_000 + 50_000n

Method
  1_000_000 + 100_000n

This change updates codeclimate-rubocop to be closer to those standards.

Code Climate users can turn on one or all complexity checks, with the
understanding that there may be overlap between them.

Users can also configure thresholds in their .rubocop.yml file.

@ABaldwinHunter ABaldwinHunter force-pushed the abh-tune-remediation-points branch 2 times, most recently from 4c20d34 to 731862a Compare December 28, 2015 16:07
@ABaldwinHunter ABaldwinHunter mentioned this pull request Dec 28, 2015
Merged
@ABaldwinHunter
Copy link
Contributor Author

Ready for re-review.

@ABaldwinHunter
Copy link
Contributor Author

cc @codeclimate/review

@ABaldwinHunter ABaldwinHunter force-pushed the abh-tune-remediation-points branch from 731862a to 21e346a Compare December 28, 2015 20:15
@gdiggs
Copy link
Contributor

gdiggs commented Dec 28, 2015

LGTM

brynary
brynary reviewed Dec 28, 2015
View reviewed changes
config/cops.yml
violation_points: 5_000
Metrics/MethodLength:
base_points: 50_000
base_points: 100_000
Copy link
Member

Choose a reason for hiding this comment

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

Is LineLength for a single LOC being too long (wide)? If so, this seems too high.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, concur that's the offense.

Reverting to the current setting:

Metrics/LineLength:
  base_points: 20_000
  violation_points: 5_000

if that sounds good?

Copy link
Member

Choose a reason for hiding this comment

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

So for a style thing (like missing semicolon in JS) we do 50k. I think that applies here as well. I'm not sure anything would ever be less than 50k, since the 50k number is intended to account for the time it takes to open the file, make the change, save, commit, push... it's about 5 mins even for something small.

For a long line, it doesn't seem to get harder to fix as the line gets longer. Therefore, I'd just do a flat 50k.

@brynary
Copy link
Member

brynary commented Dec 28, 2015

Reviewed again. Had one question about LineLength (added inline). Everything else LGTM

@ABaldwinHunter ABaldwinHunter force-pushed the abh-tune-remediation-points branch from 21e346a to 17d6f9e Compare December 28, 2015 20:32
ABaldwinHunter added 2 commits December 28, 2015 17:09
Tune remediation points
63c7ac5 
Tune complexity points, creating more parity with Ruby analysis
in Code Climate classic.

- Class violations: 5_000_000 + n35_000
- Method violations: 1_000_000 + n70_000
Add nil guard in spec
342a501
@ABaldwinHunter ABaldwinHunter force-pushed the abh-tune-remediation-points branch from 17d6f9e to 342a501 Compare December 28, 2015 22:10
ABaldwinHunter pushed a commit that referenced this pull request Dec 29, 2015
Merge pull request #42 from codeclimate/abh-tune-remediation-points
f305da3 
Tune remediation points
@ABaldwinHunter ABaldwinHunter merged commit f305da3 into master Dec 29, 2015
@ABaldwinHunter ABaldwinHunter deleted the abh-tune-remediation-points branch December 29, 2015 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ABaldwinHunter @gdiggs @brynary @mrb @pbrisbin