Don't report all errors on each file analyzed #18
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
a35a8c0 to
c5f3658
Compare
| @@ -22,6 +22,12 @@ module Engine | |||
| expect{ lint.run }.to_not output.to_stdout | |||
| end | |||
|
|
|||
| it "doesn't report issues for EACH file as present in ALL files" do | |||
There was a problem hiding this comment.
Might be clearer as "only reports issues in the relevant file"?
|
LGTM |
It's likely the original developer assumed that the pattern given to
Node#xpath is treated relative to the node on which it's called. That
doesn't seem to be the case.
The XPath "//error" finds ALL error nodes across the entire document,
even when invoked as file.xpath("//error"). This causes them all to be
reported on every file analyzed. Besides being extremely non-sensical,
this can also be a performance nightmare.
To correct this, we instead use Node#children to get only the currently
processing file's errors. This required replacing some hash accesses
with methods and the chaining of #value on attributes.
c5f3658 to
e7aff4f
Compare
|
Nice find! |
|
|
pbrisbin
added a commit
that referenced
this pull request
Dec 7, 2015
Don't report all errors on each file analyzed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It's likely the original developer assumed that the pattern given to Node#xpath
is treated relative to the node on which it's called. That doesn't seem to
be the case.
The XPath "//error" finds ALL error nodes across the entire document, even when
invoked as file.xpath("//error"). This causes them all to be reported on every
file analyzed. Besides being extremely non-sensical, this can also be a
performance nightmare.
To correct this, we instead use Node#children to get only the currently
processing file's errors. This required replacing some hash accesses with
methods and the chaining of #value on attributes.
/cc @codeclimate/review