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 all commits
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,10 +63,23 @@ 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 | ||
NO_LOCATION = PseudoSourceRange.new(1, 0, '', 0, 0).freeze | ||
|
||
# @api private | ||
def initialize(severity, location, message, cop_name, # rubocop:disable Metrics/ParameterLists | ||
|
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: 0, | ||
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