Skip to content

Report issue for each occurrence of duplication #83

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.

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

Merged
merged 1 commit into from
Jan 26, 2016

Conversation

ABaldwinHunter
Copy link
Contributor

This change fixes a bug in which we reported only the first occurrence
(or sexp) of duplicated code in a set, and dropped the rest. The others
were included in other_locations but not reported as an independent issue.

@codeclimate/review
cc @pbrisbin

First step in breaking apart #78

description = "#{check_name} found in #{occurrences} other location"
description += "s" if occurrences > 1
description = "#{check_name} found in #{(other_sexps.length)} other location"
description += "s" if other_sexps.length > 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we preserved #occurrences, I think you can revert this too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@ABaldwinHunter ABaldwinHunter force-pushed the abh-pb-multi-issues branch 6 times, most recently from 0e25163 to b0e24ae Compare January 26, 2016 00:10
@ABaldwinHunter
Copy link
Contributor Author

@pbrisbin Ready for re-review!

@ABaldwinHunter
Copy link
Contributor Author

Removed any drive-by changes and added spec.

@@ -93,6 +95,7 @@

result = run_engine(engine_conf).strip
issues = result.split("\0")

Copy link
Contributor

Choose a reason for hiding this comment

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

Accidental change.

This change fixes a bug in which we reported only the first occurrence
(or sexp) of duplicated code in a set, and dropped the rest. The others
were included in other_locations but not reported as an independent issue.
@pbrisbin
Copy link
Contributor

This LGTM.

I fully expect this to have a (potentially strong) impact on grades. We're taking what was a single thing representing N instances of duplication with X remediation_points and now reporting N things each with X remediation_points. This means the total remediation_points for the duplication will go from X to X*N. I'm fine with this, we'll discuss it and tune points and grades separately via a new PR, but it should be explained in the ChangeLog.

@ABaldwinHunter
Copy link
Contributor Author

👍

The mass and point algorithm changes coming on the heels of this PR will also impact grades. The overall impact may be smaller grade impact on more files.

ABaldwinHunter pushed a commit that referenced this pull request Jan 26, 2016
Report issue for each occurrence of duplication
@ABaldwinHunter ABaldwinHunter merged commit a9af216 into master Jan 26, 2016
@ABaldwinHunter ABaldwinHunter deleted the abh-pb-multi-issues branch January 26, 2016 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants