Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SourceFingerprint based on path, check_name, and source #419

Merged
merged 1 commit into from
May 5, 2016

Conversation

dblandin
Copy link
Contributor

@dblandin dblandin commented Apr 22, 2016

Currently, the default fingerprint for an issue is computed by creating an MD5 digest based on the path of the file which introduced the issue and the check name for the issue emitted.

The result of this fingerprint for many engines is that issues emitted with the same check name for the same file are given the same fingerprint. On codeclimate.com, we only show issues with unique fingerprints, so even if 100 issues were emitted for a file, if all issues share the same check name and thus the same fingerprint, we only show one issue.

Additionally, a fixed or introduced issue is often ignored if there are current issues with the same fingerprint already on the default branch.

This PR introduces new fingerprinting behavior to address these issues, first added to a couple engines, that incorporates relevant source code into the fingerprint. This implementation allows us to enable the new fingerprinting behavior with an environment variable so we can roll it out gradually. Eventually, this will become the new default fingerprinting method for engines.

@codeclimate/review 馃攷

@dblandin dblandin force-pushed the devon/add-source-fingerprinting branch from b0b14d8 to 3abcc73 Compare April 22, 2016 19:28
if ENV["CODECLIMATE_SOURCE_FINGERPRINT"]
::CC::CLI::SourceFingerprint.new(parsed_output).compute
else
::CC::CLI::PathFingerprint.new(parsed_output).compute
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is telling you these objects should live under the Analyzer namespace. Is there a reason you went with CLI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, that was an oversight. I'll move it into Analyzer.

@dblandin dblandin force-pushed the devon/add-source-fingerprinting branch 7 times, most recently from d1127a7 to 791e545 Compare April 26, 2016 20:09
@dblandin
Copy link
Contributor Author

@wfleming I pushed up a commit with support for location positions either in the line/column or offset form. Let me know what you think.

@dblandin dblandin force-pushed the devon/add-source-fingerprinting branch 2 times, most recently from 0a28d48 to 347d7df Compare April 26, 2016 20:15
@dblandin
Copy link
Contributor Author

@codeclimate/review This is ready for another look! I addressed all of the comments brought up in the last round.

offset += column
else
offset += source_line.length
end
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW: I've started to come around to:

offset +=
  if line == index
    column
  else
    source_line.length
  end

Which appeases rubocop without misaligning the if/else/end depending on the size of the LHS of the assignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks a little strange, but seems better to me! 馃憤

@pbrisbin
Copy link
Contributor

Some minor comments. Looks good enough to RC and move onto a builder branch IMO.

@dblandin dblandin force-pushed the devon/add-source-fingerprinting branch 4 times, most recently from 8b7ff64 to b9bedd0 Compare April 27, 2016 15:18
@dblandin dblandin force-pushed the devon/add-source-fingerprinting branch from b9bedd0 to a457065 Compare April 27, 2016 15:20
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.

None yet

5 participants