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

Suppress SyntaxWarnings when parsing modules #2386

Merged
merged 4 commits into from
Feb 19, 2024

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Feb 19, 2024

Description

Following the conclusion in pylint-dev/pylint#9322, ignore SyntaxWarnings when parsing modules with ast.parse.

@cdce8p cdce8p added this to the 3.1.0 milestone Feb 19, 2024
@jacobtylerwalls
Copy link
Member

What do you think about ignoring SyntaxWarning only if SyntaxWarning is missing from the warning filters registry, as it is by default? That would allow users to exercise control over the warnings.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 19, 2024

What do you think about ignoring SyntaxWarning only if SyntaxWarning is missing from the warning filters registry, as it is by default? That would allow users to exercise control over the warnings.

Not sure it's worth the effort. For Home Assistant the solution should be start linting all requirements as well.
What I don't like about it currently is that modifying the warnings filter for each file is quite a performance hit. Maybe it's better to do that globally?

Copy link

codecov bot commented Feb 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e26f097) 92.75% compared to head (591c848) 92.75%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2386   +/-   ##
=======================================
  Coverage   92.75%   92.75%           
=======================================
  Files          94       94           
  Lines       11066    11070    +4     
=======================================
+ Hits        10264    10268    +4     
  Misses        802      802           
Flag Coverage Δ
linux 92.56% <100.00%> (+<0.01%) ⬆️
pypy 90.88% <75.00%> (-0.01%) ⬇️
windows 92.34% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
astroid/builder.py 94.22% <100.00%> (+0.10%) ⬆️

ChangeLog Outdated Show resolved Hide resolved
Co-authored-by: Jacob Walls <[email protected]>
@cdce8p
Copy link
Member Author

cdce8p commented Feb 19, 2024

It doesn't seem to make a difference performance wise. The first observation was probably a mistake. Never the less, I do like this version a bit more if I'm honest.

The PR would be ready IMO.

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Great, thank you!

Next step would be to update pylint-dev/pylint@2034874 as part of the astroid 3.1 upgrade.

@Pierre-Sassoulas do you want to move forward on cutting astroid 3.1? I think leaving the numpy workaround in #2375 is alright for now.

@cdce8p cdce8p merged commit 07419f0 into pylint-dev:main Feb 19, 2024
20 checks passed
@cdce8p cdce8p deleted the ignore-syntax-warnings branch February 19, 2024 13:52
@cdce8p
Copy link
Member Author

cdce8p commented Feb 22, 2024

@Pierre-Sassoulas do you want to move forward on cutting astroid 3.1?

I think a new release would make sense. This, together with the fix for the ValueError, is one of the last blockers before I can upgrade Home Assistant to use 3.12 for the pylint check.

@Pierre-Sassoulas
Copy link
Member

Sure we were planning on releasing with Jacob. I was trying to find the time to check numpy pyi consistency against our astroid numpy's brain. (I don't remember the issue and I'm on mobile). It Can probably be dealt with later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants