Skip to content
This repository was archived by the owner on Jul 19, 2025. It is now read-only.

Conversation

gdiggs
Copy link
Contributor

@gdiggs gdiggs commented Nov 10, 2015

This reverts the previous attempt to exclude the current snippet from other_locations.
It's not always the first in hashes, but current_sexp uses the first
of the sorted array, so dropping that one is safe.

Covered by tests here:
https://github.com/codeclimate/codeclimate-duplication/blob/master/spec/cc/engine/analyzers/javascript/main_spec.rb#L41-42

@codeclimate/review

It's not always the first in `hashes`, but `current_sexp` uses the first
of the sorted array, so dropping that one is safe.

Covered by tests here:
https://github.com/codeclimate/codeclimate-duplication/blob/master/spec/cc/engine/analyzers/javascript/main_spec.rb#L41-42
@gdiggs gdiggs changed the title Gd other locations Don't show snippet in other locations Nov 10, 2015
@@ -33,7 +32,7 @@ def report_name

private

attr_reader :base_points, :hashes, :reports
attr_reader :base_points, :hashes

def current_sexp
@location ||= sorted_hashes.first
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 is where it uses sorted_hashes

@gdiggs
Copy link
Contributor Author

gdiggs commented Nov 10, 2015

screen shot 2015-11-10 at 11 29 37 am

Sample result

@BlakeWilliams
Copy link
Contributor

Out of curiosity, can you try this with two duplicated lines of JSX?

 <a className='button button-primary full' href='#' onClick={this.onSubmit.bind(this)}>Login</a>

I think since that has multiple duplications in the same line other_locations might get triggered, but I'm not sure.

@BlakeWilliams
Copy link
Contributor

LGTM

@jpignata
Copy link
Contributor

👍

gdiggs added a commit that referenced this pull request Nov 10, 2015
Don't show snippet in other locations
@gdiggs gdiggs merged commit 91af8e9 into master Nov 10, 2015
@gdiggs gdiggs deleted the gd-other-locations branch November 10, 2015 16:37
@gdiggs
Copy link
Contributor Author

gdiggs commented Nov 10, 2015

@BlakeWilliams pasting that line twice worked correctly with these changes. Though, apparently 2 lines like that without a wrapping div is invalid JSX

@BlakeWilliams
Copy link
Contributor

Cool, good to know. And yeah, JSX requires one top-level div.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants