Add end_lineno and end_col_offset to MessageLocationTuple#5343
Add end_lineno and end_col_offset to MessageLocationTuple#5343DanielNoord merged 3 commits intopylint-dev:mainfrom
end_lineno and end_col_offset to MessageLocationTuple#5343Conversation
Pull Request Test Coverage Report for Build 1490926947
💛 - Coveralls |
Pierre-Sassoulas
left a comment
There was a problem hiding this comment.
Looks promising, I think there's also the functional tests to take into account and the output txt serialization/deserialization from testutil to upgrade ? Also maybe it's time to introduce a location class with lineno, column, lineno_end, column_end attributes?
I want to tackle those in another PR after we can actually access these attributes.
That's what |
|
Should I fix the |
Yes, in order to avoid a double conflict when rebasing on master. We'll have to ask every contributor that touched the functional tests to rebase to get end_line / end_column for some time after merging this. |
| "end_lineno", | ||
| "end_col_offset", |
There was a problem hiding this comment.
Not a full review yet.
I was wondering, should we rename those to end_line and end_column respectively? That would match the existing line and column. Relevant as these are the arguments to --msg-template.
There was a problem hiding this comment.
I opted to follow what ast uses, but yeah this makes sense. I'll change!
|
I'm not sure what tests we would like to add here. We actually only have one test for the I propose to merge this PR and then make three separate PRs, based on the original to-do list:
Only 1 is necessary for the beta release of this functionality, although if we manage to do so I think it would be nice to put them all in the same release. |
Pierre-Sassoulas
left a comment
There was a problem hiding this comment.
I propose to merge this PR and then make three separate PRs, based on the original to-do list
👍
cdce8p
left a comment
There was a problem hiding this comment.
Ok lets try that! It at least seems to work with vscode-python. Although I'm still a bit unsure about the None values.
| line or 1, | ||
| col_offset or 0, | ||
| end_lineno, | ||
| end_col_offset, |
There was a problem hiding this comment.
I was thinking again about this. Although I'm not 100% there yet, setting end_lineno and end_col_offset to None seems like the best we can do.
We probably shouldn't touch line or 1 and col_offset or 0 though as this will break tools.
|
@DanielNoord I let you merge when it's ready :) |
Type of Changes
Description
Ref: #5336
Preparation for when
astroidallows us to actually get these attributes. This allows us to start passing the needed attributes to reporters. Some points for consideration:I have opted to type these as
Optional[int]. I think it might be more valuable to know that a certain message does not have an associatedend_lineorend_col_offsetinstead of an arbitrarily chosen one. If we want to pass an arbitrary number we can still can, but at least now we don't need to do so.I looked online but couldn't find an answer: is it possible to let users using
MessageLocationTupleknow that they should supply aend_lineandend_col_offset? I think it is better to require users to think about whether they wantend_lineto beNoneat creation time (when they still have access to thenode) then through the default value. Personally I would like these to be required inpylint3.0, but couldn't find a good way to deprecateMessageLocationTuplewith 6 values.The necessity of using default values for
MessageLocationTuplealso makes this requirepython>3.6.0. I'm not sure what the idea behind2.12was: do we allow >3.6.0functions in it? Or do we try to minimise them?