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

Prefer .pyi stubs #2375

Merged
merged 7 commits into from
May 6, 2024
Merged

Conversation

jacobtylerwalls
Copy link
Member

Type of Changes

Type
✨ New feature

Description

Closes pylint-dev/pylint#9185

@jacobtylerwalls jacobtylerwalls added the Enhancement ✨ Improvement to a component label Feb 4, 2024
@jacobtylerwalls jacobtylerwalls added this to the 3.1.0 milestone Feb 4, 2024
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 like a clever way of doing thing. Regarding the error I have an intuition that our (now outdated/wrong) brain / tests and the actual content of the numpy pyi are conflicting which would be a good thing, but I don't have the time to test that hypothesis right now.

tests/brain/test_attr.py Show resolved Hide resolved
tests/brain/test_attr.py Show resolved Hide resolved
astroid/interpreter/_import/spec.py Outdated Show resolved Hide resolved
@jacobtylerwalls
Copy link
Member Author

but I don't have the time to test that hypothesis right now.

Neither do I. It's not straightfoward. A soft launch of this without numpy seems wise. Pushed an ugly workaround.

Copy link

codecov bot commented Feb 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.78%. Comparing base (4a8827d) to head (758f4d8).
Report is 4 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2375      +/-   ##
==========================================
+ Coverage   92.76%   92.78%   +0.01%     
==========================================
  Files          94       94              
  Lines       11090    11098       +8     
==========================================
+ Hits        10288    10297       +9     
+ Misses        802      801       -1     
Flag Coverage Δ
linux 92.59% <85.71%> (+0.01%) ⬆️
pypy 92.78% <100.00%> (+0.01%) ⬆️
windows 92.68% <100.00%> (+0.01%) ⬆️

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

Files Coverage Δ
astroid/interpreter/_import/spec.py 97.47% <100.00%> (+0.07%) ⬆️
astroid/modutils.py 88.15% <100.00%> (+0.04%) ⬆️

... and 4 files with indirect coverage changes

@Pierre-Sassoulas
Copy link
Member

Yeah it would work 😄 But if we're right and our tests are actually outdated (high possibility seeing they are very old and numpy is under heavy development), we're going to prevent numpy from having this new feature. Tbf I did not intend to check everything just check one test to confirm or not. Trying to find the motivation to download numpy and start reading atm. (Also we have a ton of brain for numpy, I those brains could become a pylint-numpy plugin so we don't have to block astroid for lib specific behavior) .

@jacobtylerwalls
Copy link
Member Author

@Pierre-Sassoulas do you think we can get this into astroid 3.2 as is?

@jacobtylerwalls
Copy link
Member Author

jacobtylerwalls commented May 4, 2024

pylint primer result

@Pierre-Sassoulas
Copy link
Member

Yeah we can release as is and not make astroid worse than it is at the moment. I keep this on my mind but handling the flow of issues and then, at the end of a big pile of issues, finding the motivation to open numpy's code and compare it to the astroid's numpy brains and unit tests ..is proving difficult. Might need to create an issue for the numpy / pyi / outdated tests checks in the end.

@Pierre-Sassoulas
Copy link
Member

Or a pylint numpy plugin.

@jacobtylerwalls
Copy link
Member Author

I hear you. Plus, what if we get negative user-feedback on this first push -- don't want to sink time into a big numpy yak shave that turns out to be wrong-track!

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.

Some nits :)

ChangeLog Outdated Show resolved Hide resolved
astroid/interpreter/_import/spec.py Outdated Show resolved Hide resolved
jacobtylerwalls and others added 2 commits May 6, 2024 08:02
Co-authored-by: Pierre Sassoulas <[email protected]>
Co-authored-by: Pierre Sassoulas <[email protected]>
@jacobtylerwalls jacobtylerwalls merged commit 0984386 into pylint-dev:main May 6, 2024
20 checks passed
@jacobtylerwalls jacobtylerwalls deleted the prefer-pyi branch May 6, 2024 12:36
@mbyrnepr2
Copy link
Member

Or a pylint numpy plugin.

Should that be standalone and a dependency for pylint-ml?

@Pierre-Sassoulas
Copy link
Member

Yeah that sounds good but we'd need a proof of concept first. Probably on a smaller lib with less users affected if we drop the brain from astroid/pylint by default to test the water.

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.

.py files having precedence over .pyi files causes problems with opencv-python module (and probably others)
4 participants