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
[Fix #8718] Fix undefined methods of pseudo location #8823
Changes from 1 commit
f87c817
7e3b3da
696a076
64ec21b
075c98e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,7 +63,20 @@ class Offense | |
attr_reader :corrector | ||
|
||
PseudoSourceRange = Struct.new(:line, :column, :source_line, :begin_pos, | ||
:end_pos) | ||
:end_pos) do | ||
alias_method :first_line, :line | ||
alias_method :last_line, :line | ||
alias_method :last_column, :column | ||
|
||
def column_range | ||
column...last_column | ||
end | ||
|
||
def size | ||
end_pos - begin_pos | ||
end | ||
alias_method :length, :size | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [note] This redefines There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's ok. |
||
end | ||
private_constant :PseudoSourceRange | ||
|
||
NO_LOCATION = PseudoSourceRange.new(1, 0, '', 0, 1).freeze | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -180,4 +180,49 @@ def Foo | |
expect(offense.highlighted_area.source).to eq('Foo') | ||
end | ||
end | ||
|
||
context 'when the location is pseudo' do | ||
let(:location) { described_class::NO_LOCATION } | ||
|
||
it 'returns a location with valid size and length' do | ||
expect(offense.location.size).to eq 1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't remember at all why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The following PR seems the origin of https://github.com/rubocop-hq/rubocop/pull/2516/files#diff-4f6676361bec108acba5cfd5a6d979b3 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you could try with 0, I think it would make more sense (especially on an empty file!) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I try it via 7e3b3da. |
||
expect(offense.location.length).to eq 1 | ||
end | ||
|
||
it 'returns a line' do | ||
expect(offense.line).to eq 1 | ||
end | ||
|
||
it 'returns a column' do | ||
expect(offense.column).to eq 0 | ||
end | ||
|
||
it 'returns a source line' do | ||
expect(offense.source_line).to eq '' | ||
end | ||
|
||
it 'returns a column length' do | ||
expect(offense.column_length).to eq 0 | ||
end | ||
|
||
it 'returns the first line' do | ||
expect(offense.first_line).to eq 1 | ||
end | ||
|
||
it 'returns the last line' do | ||
expect(offense.last_line).to eq 1 | ||
end | ||
|
||
it 'returns the last column' do | ||
expect(offense.last_column).to eq 0 | ||
end | ||
|
||
it 'returns a column range' do | ||
expect(offense.column_range).to eq 0...0 | ||
end | ||
|
||
it 'returns a real column' do | ||
expect(offense.real_column).to eq 1 | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -157,5 +157,21 @@ | |
it 'sets length value for :length key' do | ||
expect(hash[:length]).to eq(8) | ||
end | ||
|
||
context 'when the location is pseudo' do | ||
let(:location) { RuboCop::Cop::Offense::NO_LOCATION } | ||
|
||
it 'returns a valid hash' do | ||
expect(hash).to eq({ | ||
start_line: 1, | ||
start_column: 1, | ||
last_line: 1, | ||
last_column: 0, | ||
length: 1, | ||
line: 1, | ||
column: 1 | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [note] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does seem a little odd, however the pseudo source range is also not exactly accurate compared to other offenses since it is used to identify offenses for a file rather than an actual source range. I think it is OK for there to be a bit of non perfect behavior at the moment. It does kind of seem like there might be reason to do a larger refactor to better handle offenses in a file vs offenses in source code. Maybe the offense itself shouldn't be responsible for the column information, and this kind of info should be handled in the location. I'm not sure how large of a refactor this would take. Similarly, does it make sense to be setting the line number of the offense to 1 for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rrosenblum I agree with your idea, thank you. Surely, in this PR, it seems good that we avoid the big change of the current behavior. But for the benefit of others, here is an example of comparison: Current (line=1, col=1):
Maybe (line=0, col=0):
I will follow the decision of the community. |
||
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[note] See https://github.com/whitequark/parser/blob/v2.7.1.5/lib/parser/source/range.rb#L114-L120