Skip to content
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

Add end_lineno and end_col_offset to MessageLocationTuple #5343

Merged
merged 3 commits into from
Nov 22, 2021

Conversation

DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Write a good description on what the PR does.

Type of Changes

Type
✨ New feature

Description

Ref: #5336

Preparation for when astroid allows us to actually get these attributes. This allows us to start passing the needed attributes to reporters. Some points for consideration:

  1. 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 associated end_line or end_col_offset instead 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.

  2. I looked online but couldn't find an answer: is it possible to let users using MessageLocationTuple know that they should supply a end_line and end_col_offset? I think it is better to require users to think about whether they want end_line to be None at creation time (when they still have access to the node) then through the default value. Personally I would like these to be required in pylint 3.0, but couldn't find a good way to deprecate MessageLocationTuple with 6 values.

  3. The necessity of using default values for MessageLocationTuple also makes this require python > 3.6.0. I'm not sure what the idea behind 2.12 was: do we allow > 3.6.0 functions in it? Or do we try to minimise them?

@DanielNoord DanielNoord added Enhancement ✨ Improvement to a component High priority Issue with more than 10 reactions labels Nov 19, 2021
@coveralls
Copy link

coveralls commented Nov 19, 2021

Pull Request Test Coverage Report for Build 1490926947

  • 17 of 17 (100.0%) changed or added relevant lines in 4 files are covered.
  • 44 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.01%) to 93.446%

Files with Coverage Reduction New Missed Lines %
pylint/config/option_manager_mixin.py 12 89.2%
pylint/lint/pylinter.py 32 94.23%
Totals Coverage Status
Change from base Build 1487596931: 0.01%
Covered Lines: 13931
Relevant Lines: 14908

💛 - Coveralls

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@DanielNoord
Copy link
Collaborator Author

Looks promising, I think there's also the functional tests to take into account and the output txt serialization/deserialization from testutil to upgrade ?

I want to tackle those in another PR after we can actually access these attributes.

Also maybe it's time to introduce a location class with lineno, column, lineno_end, column_end attributes?

That's what MessageLocationTuple is for :) We actually implemented that a couple of weeks ago!

@DanielNoord
Copy link
Collaborator Author

Should I fix the TODO in this PR and actually access NodeNG.end_line and NodeNG.end_col_offset or do we want to merge this first and keep the PRs small and manageable?

@Pierre-Sassoulas
Copy link
Member

I fix the TODO in this PR and actually access NodeNG.end_line and NodeNG.end_col_offset

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.

Comment on lines 28 to 29
"end_lineno",
"end_col_offset",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted to follow what ast uses, but yeah this makes sense. I'll change!

@DanielNoord
Copy link
Collaborator Author

DanielNoord commented Nov 22, 2021

I'm not sure what tests we would like to add here. We actually only have one test for the Message class which gets updated in this PR.
Let me know if you want to add any additional.

I propose to merge this PR and then make three separate PRs, based on the original to-do list:

  1. Modify reporters to accept end_line and end_column -> Update reporters to (allow) use of end_line and end_column #5372
  2. Modify functional test runner to accept end_line and end_column with a DeprecationWarning if not supplied -> Update functional test expected output #5349
  3. Modify unittests to use end_line and end_column

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.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose to merge this PR and then make three separate PRs, based on the original to-do list

👍

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.12.0 milestone Nov 22, 2021
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok lets try that! It at least seems to work with vscode-python. Although I'm still a bit unsure about the None values.

Comment on lines +1481 to +1484
line or 1,
col_offset or 0,
end_lineno,
end_col_offset,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Pierre-Sassoulas
Copy link
Member

@DanielNoord I let you merge when it's ready :)

@DanielNoord DanielNoord merged commit ee70d41 into pylint-dev:main Nov 22, 2021
@DanielNoord DanielNoord deleted the column branch November 22, 2021 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component High priority Issue with more than 10 reactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants