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 test for relative-beyond-top-level false positive #5059

Merged
merged 2 commits into from
Oct 17, 2021

Conversation

DanielNoord
Copy link
Collaborator

@DanielNoord DanielNoord commented Sep 21, 2021

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

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

This builds upon the fix and later regression fixes introduced in pylint-dev/astroid#1186, pylint-dev/astroid#1200, pylint-dev/astroid#1211

I think running pylint on all the files in the test folder might be a bit overkill but I wanted to show my working solution and get feedback first. As said in the astroid PR I'm not 100% comfortable with namespace packages and importing, but this seems to fix the issue!

Closes #2967
Closes #5131

@DanielNoord
Copy link
Collaborator Author

Do we want to add the extra tests added to astroid for this change to be added to pylint as well?

@coveralls
Copy link

coveralls commented Oct 7, 2021

Pull Request Test Coverage Report for Build 1350996452

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 93.236%

Totals Coverage Status
Change from base Build 1350956681: 0.01%
Covered Lines: 13633
Relevant Lines: 14622

πŸ’› - Coveralls

@DanielNoord DanielNoord removed the Needs astroid update Needs an astroid update (probably a release too) before being mergable label Oct 7, 2021
@Pierre-Sassoulas
Copy link
Member

Do we want to add the extra tests added to astroid for this change to be added to pylint as well?

Well you're right it's notre strictly necessary, but pylint is modifying the path and doing complex thing on top of astroid generally so I would deem keeping them prudent.

@cdce8p
Copy link
Member

cdce8p commented Oct 7, 2021

@DanielNoord Can you add the example from pylint-dev/astroid#1200 here as well?

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.

Sorry I forgot that we need an entry in the changelog.

@DanielNoord
Copy link
Collaborator Author

@DanielNoord Can you add the example from PyCQA/astroid#1200 here as well?

Done!

Sorry I forgot that we need an entry in the changelog.

Do we? This was a change in astroid without any code changes in pylint, only tests. I know for a similar case we didn't add anything.

@cdce8p
Copy link
Member

cdce8p commented Oct 7, 2021

@DanielNoord Can you add the example from PyCQA/astroid#1200 here as well?

Done!

Thanks!

Sorry I forgot that we need an entry in the changelog.

Do we? This was a change in astroid without any code changes in pylint, only tests. I know for a similar case we didn't add anything.

It's not strictly required, but probably better. Don't know how many users will actually read the astroid changelog. Just a one liner + 'closes' should be enough though. It might work to reuse the astroid changelog entry for that. (That's what I did before merging your requires astroid update PRs today.)

@DanielNoord
Copy link
Collaborator Author

Hm, tests pass on Mac, but fail on windows...
If anybody has any good clues as to why this might be happening, let me know!

@DanielNoord
Copy link
Collaborator Author

Can we merge this before we get to the bottom of #5131? Or do we want to wait for that issue as well?

@Pierre-Sassoulas
Copy link
Member

I think we'll need to add a regression test for the other issue before 2.12 goes out but we don't have to do it in this PR necesssarily. I don't fear conflicts if we don't merge this immediately though.

@DanielNoord DanielNoord added the Needs astroid update Needs an astroid update (probably a release too) before being mergable label Oct 12, 2021
@DanielNoord
Copy link
Collaborator Author

This should be fixed by pylint-dev/astroid#1211

See explanation of the latest regression there.

ChangeLog Outdated Show resolved Hide resolved
@DanielNoord DanielNoord deleted the namespace branch October 17, 2021 19:04
@DanielNoord
Copy link
Collaborator Author

At some point we might need to check if the changes to the import resolver actually fixed some of the reported issues with namespace packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug πŸͺ² Import system Needs astroid update Needs an astroid update (probably a release too) before being mergable
Projects
None yet
4 participants