Skip to content

Use node.position in add_message if available #5897

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

Merged
merged 5 commits into from
Mar 12, 2022

Conversation

DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature, or an important bug fix, add a What's New entry in
    doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Type of Changes

Type
✨ New feature

Description

Closes #5466.

I'd like to get approval on the code before I do the automatic updating of the tests. So please review this already even if tests fail 😄

@DanielNoord DanielNoord added Enhancement ✨ Improvement to a component Blocker 🙅 Blocks the next release labels Mar 11, 2022
@DanielNoord DanielNoord added this to the 2.13.0 milestone Mar 11, 2022
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.

The current change LGTM :)

@DanielNoord DanielNoord added the Needs astroid update Needs an astroid update (probably a release too) before being mergable label Mar 11, 2022
@cdce8p cdce8p marked this pull request as draft March 11, 2022 15:12
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.

The current change LGTM :)

👍🏻

@DanielNoord DanielNoord marked this pull request as ready for review March 12, 2022 13:01
@DanielNoord DanielNoord removed the Needs astroid update Needs an astroid update (probably a release too) before being mergable label Mar 12, 2022
@DanielNoord
Copy link
Collaborator Author

@Pierre-Sassoulas Let's merge this first before we add all the regression test PRs. Otherwise we need to keep rebasing this.

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.

Missed that earlier, do we have an option for error messages to either opt out of or opt into using the position attribute? Say for example one message should be emitted for the whole function and not just the def <name>. Maybe a new parameter for add_message?

@DanielNoord
Copy link
Collaborator Author

Missed that earlier, do we have an option for error messages to either opt out of or opt into using the position attribute? Say for example one message should be emitted for the whole function and not just the def <name>. Maybe a new parameter for add_message?

If you supply line etc. when you add the message nothing will be overwritten. Is that enough?

Note: apparently 3.8 is broken... I'll merge the other test PRs first while working on this.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1973639791

  • 34 of 34 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 94.044%

Totals Coverage Status
Change from base Build 1973532638: 0.02%
Covered Lines: 15049
Relevant Lines: 16002

💛 - 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.

Thank you @DanielNoord !

@Pierre-Sassoulas Pierre-Sassoulas merged commit 6ead5fd into pylint-dev:main Mar 12, 2022
@DanielNoord DanielNoord deleted the position branch March 12, 2022 18:28
@DanielNoord DanielNoord mentioned this pull request Mar 12, 2022
2 tasks
@cdce8p cdce8p removed the Blocker 🙅 Blocks the next release label Mar 13, 2022
@cdce8p
Copy link
Member

cdce8p commented Mar 13, 2022

Missed that earlier, do we have an option for error messages to either opt out of or opt into using the position attribute? Say for example one message should be emitted for the whole function and not just the def <name>. Maybe a new parameter for add_message?

If you supply line etc. when you add the message nothing will be overwritten. Is that enough?

My initial idea was a parameter to toggle between both options. Not passing each value individually. However, after comparing the changes here, I think it might no longer be necessary. So far I haven't found a case were it would be noticeable better to mark the whole code block. Let's leave it as it is for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better error ranges for classes and functions
4 participants