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

Fix relative-beyond-top-level false positive #1186

Merged
merged 5 commits into from
Oct 5, 2021

Conversation

DanielNoord
Copy link
Collaborator

@DanielNoord DanielNoord commented Sep 21, 2021

Steps

  • For new features or bug fixes, add a ChangeLog entry describing what your PR does.
  • Write a good description on what the PR does.

Description

I believe this solves the false positive as discussed in the pylint issue. However, I'm not 100% comfortable with namespace packages so please tell me if I did anything wrong here.

I have not added a test yet, but I have openend a pylint PR which includes a test that does pass.
See: pylint-dev/pylint#5059

Type of Changes

Type
🐛 Bug fix

Related Issue

pylint-dev/pylint#2967

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 think we need to add a test in astroid too. Maybe we can use the same one than in pylint ?

@DanielNoord
Copy link
Collaborator Author

@Pierre-Sassoulas Would you mind taking a look at this? I have added the necessary files to create the same test as in pylint but I don't really understand how the unit tests with data work in astroid.

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Sep 26, 2021

I don't really understand how the unit tests with data work in astroid.

There's no handy automated functional tests like in pylint :) I think you have to parse them manually, there's probably example of reading data in the testdata directory Maybe we could imagine adding some helper like in pylint but we'd have to create a text representation of the expected AST (?) This would be helpful but probably quite a lot of work too.

@DanielNoord
Copy link
Collaborator Author

I used a unittest in pylint as well, but it was much easier to understand how I could load extra data.
Searching for some of the directories I found in testdata I did not find any hits within the tests. So I couldn't find any examples I could copy.

@Pierre-Sassoulas
Copy link
Member

@hippo91 how do you handle test using real file in testdata generally ?

@hippo91
Copy link
Contributor

hippo91 commented Sep 27, 2021

@Pierre-Sassoulas @DanielNoord i never had to use testdata. However it seems that one way is to use the resources module.
I have to look a bit deeper to have an idea of how to use it for this specific PR.

@DanielNoord
Copy link
Collaborator Author

Sorry for the rebase. I have added a test based on some of the others I saw using the resources module.

@Pierre-Sassoulas Pierre-Sassoulas merged commit 1437e73 into pylint-dev:main Oct 5, 2021
@DanielNoord DanielNoord deleted the namespace branch October 5, 2021 16:30
@cdce8p
Copy link
Member

cdce8p commented Oct 5, 2021

This PR sees to cause a regression with the too-many-function-args warning for Home Assistant.
I'll need some more time to isolate the issue before posting code to reproduce it.

@Pierre-Sassoulas Pierre-Sassoulas mentioned this pull request Oct 5, 2021
2 tasks
cdce8p added a commit to cdce8p/astroid that referenced this pull request Oct 5, 2021
@cdce8p cdce8p mentioned this pull request Oct 5, 2021
@cdce8p
Copy link
Member

cdce8p commented Oct 5, 2021

Opened #1200 with an example to reproduce the error.

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

Successfully merging this pull request may close these issues.

4 participants