improve derivation of lines from Sexp #44
Conversation
| other_location = violation.send(:format_other_locations)[0] | ||
|
|
||
| expect(location).not_to eq other_location | ||
| expect(location[:lines]).to eq({:begin => 3, :end => 7}) |
There was a problem hiding this comment.
The overlap between these two nodes is a little weird, but makes sense from a parser sense: syntactically, a rescue expr doesn't end until the end: the next rescue is a distinct node, but the two do overlap in terms of lines.
I've opted not to do anything about that right now because 1) it's not a super common case for code duplication and 2) I don't have a good answer for a generalizable automatic remediation. We could just drop the "overlapping" lines from the bigger node (i.e. the above location would become {:begin => 3, :end => 4}, but I'm not confident there aren't other cases of node overlap that would result in even more nonsensical display with that behavior.
wfleming
force-pushed
the
will/issue-location
branch
2 times, most recently
from
42a8ec7 to
9ea5df5
Compare
|
@pbrisbin refactored |
| end | ||
|
|
||
| def to_h | ||
| {"begin": begin_line, "end": end_line} |
There was a problem hiding this comment.
What do you think about leaving the hashifiying to #format in the other class? Feels like a "formatting" responsibility.
Then you can feel better testing the public begin_line/end_line methods that indeed need to be exposed.
There was a problem hiding this comment.
I did this just because it made Violation#format_sexp prettier. Can back out.
wfleming
force-pushed
the
will/issue-location
branch
from
9ea5df5 to
7c58254
Compare
|
|
||
| module CC::Engine::Analyzers | ||
| RSpec.describe SexpLines do | ||
| let(:source) do |
There was a problem hiding this comment.
WDYT about inlining this into the example where it's used? It's only used there, and it's important to the spec, so I think it should be co-located.
|
LGTM |
wfleming
force-pushed
the
will/issue-location
branch
from
7c58254 to
e64990d
Compare
This started as a hunt into why an issue was reported for just a single line saying `end`, with the only other instance of the duplication being the same line: the source code in the spec is a reduced instance of the triggering code. The poor behavior results from how RubyParser/Sexp handle certain structures: the "line" of the `rescue..end` expression is parsed as the location of the `end`. This seems wrong, but I can't figure out a quick fix within those libraries. The way child sexprs get lines seems generally sane: at the very small level, lines seem pretty reliable. Strange behavior emerges at the higher level. The way we were calculating the end line of an sexpr already implicitly acknowledged this by walking the whole tree to find the highest line. This change adapts that previous algorithm to apply the same principle to finding both the start & end lines: the start line is basically the smallest line of the sexpr or any of its children, and the end line is the maximum.
wfleming
force-pushed
the
will/issue-location
branch
from
e64990d to
a86869c
Compare
improve derivation of lines from Sexp
This started as a hunt into why an issue was reported for just a single
line saying
end, with the only other instance of the duplication beingthe same line: the source code in the spec is a reduced instance of the
triggering code.
The poor behavior results from how RubyParser/Sexp handle certain
structures: the "line" of the
rescue..endexpression is parsed as thelocation of the
end. This seems wrong, but I can't figure out a quickfix within those libraries.
The way child sexprs get lines seems generally sane: at the very small level,
lines seem pretty reliable. Strange behavior emerges at the higher
level. The way we were calculating the end line of an sexpr already
implicitly acknowledged this by walking the whole tree to find the
highest line.
This change adapts that previous algorithm to apply the same principle
to finding both the start & end lines: the start line is basically the
smallest line of the sexpr or any of its children, and the end line is
the maximum.
👀 @codeclimate/review