Tune RUBY remediation points to match Classic #67
Conversation
ABaldwinHunter
force-pushed
the
abh-points-ruby-only
branch
2 times, most recently
from
deff4fc to
6688279
Compare
| def initialize(base_points, issue, hashes) | ||
| @base_points = base_points | ||
| def initialize(language, issue, hashes) | ||
| @language = language |
There was a problem hiding this comment.
Would calling this either strategy or language_strategy better match the variable passed in?
There was a problem hiding this comment.
I don't really understand the reasoning behind language_strategy instead of language?
There was a problem hiding this comment.
The main issue for me is the inconsistency. We're passing in an object named language_strategy in lib/cc/engine/analyzers/reporter.rb but assigning it internally within Violation as language. We should probably stick with one name.
It seems like what we're passing in, based on the methods called on the object, is more like a strategy (or configuration) than a "language".
There was a problem hiding this comment.
That is, I agree that strategy would be in keeping with the argument passed in, but I think the original name is a bit confusing.
There was a problem hiding this comment.
Hmmm, is this object always a subclass of CC::Engine::Analyzers::Base?
There was a problem hiding this comment.
Ah, missed your second comment before.
I'm super loathe to use strategy but will change it to language_strategy if you feel strongly. :)
There was a problem hiding this comment.
Now seeing that comment. Yes it is
There was a problem hiding this comment.
Then what do you think of using the name analyzer anywhere that language or language_strategy is currently used? That might clear up the confusion.
Edit: Or clean up the conflicting naming in a separate commit / PR?
There was a problem hiding this comment.
I think it's outside the scope of this PR and would advocate changing the naming in a separate refactor soon to follow.
wdyt?
|
@ABaldwinHunter This change looks good to me. Easy to follow 👍 Just a few minor nits. Are we intending to support independent point calculations in the long term or will we eventually standardize cross-language with |
|
One other question about your PR description. Is the current calculation |
|
@dblandin Thanks for feedback! I think we are going to move to For ruby, the formula is
|
ABaldwinHunter
force-pushed
the
abh-points-ruby-only
branch
2 times, most recently
from
b6581fa to
80e7ea1
Compare
Changes include:
- Refactor `calculate_points` method to be purview of language,
not `Violation` - to permit independent calculation formulas
- Increase ruby `BASE_POINTS`
- Change calculation formula from
- `base_points * issue.mass_threshold`
to
- `base_points + overage * points_per`
Rubric in classic for Ruby:
https://github.com/codeclimate/analyzer/blob/master/lib/quality/rubric.rb#L12c
Updates for other languages to come post additional language-specific tuning
ABaldwinHunter
force-pushed
the
abh-points-ruby-only
branch
from
80e7ea1 to
5a77e88
Compare
|
Updates in place! |
| def engine_conf | ||
| CC::Engine::Analyzers::EngineConfig.new({}) | ||
| def engine_conf | ||
| EngineConfig.new({}) |
There was a problem hiding this comment.
This is probably small enough now to inline on L62.
|
Looking good 👍 |
Tune RUBY remediation points to match Classic
Changes include:
calculate_pointsmethod to be purview of language,not
Violation- to permit independent calculation formulasBASE_POINTSbase_points * issue.mass_thresholdto
base_points + overage * points_perRubric in classic for Ruby:
https://github.com/codeclimate/analyzer/blob/master/lib/quality/rubric.rb#L12c
Updates for other languages to come post additional language-specific tuning
replaces https://github.com/codeclimate/codeclimate-duplication/pull/61/files
@codeclimate/review
@brynary