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

Cython pure python declare #8025

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

jtoledo1974
Copy link

Type of Changes

Type

Description

This Pull Requests provides a test case for #8024.
A pure python cython.declare(cython.int... is reported as no-member. These should be ignored.

Refs #8024

Closes #XXXX

@codecov
Copy link

codecov bot commented Jan 5, 2023

Codecov Report

Merging #8025 (416c233) into main (5612134) will increase coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8025      +/-   ##
==========================================
+ Coverage   95.42%   95.44%   +0.01%     
==========================================
  Files         176      176              
  Lines       18521    18528       +7     
==========================================
+ Hits        17674    17684      +10     
+ Misses        847      844       -3     
Impacted Files Coverage Δ
pylint/pyreverse/inspector.py 78.94% <0.00%> (ø)
pylint/checkers/variables.py 97.38% <0.00%> (+<0.01%) ⬆️
pylint/checkers/utils.py 95.33% <0.00%> (+<0.01%) ⬆️
pylint/checkers/base/basic_checker.py 97.87% <0.00%> (+0.02%) ⬆️
pylint/testutils/lint_module_test.py 86.15% <0.00%> (+1.53%) ⬆️

@jtoledo1974
Copy link
Author

@Pierre-Sassoulas , I took a look at it. Both pure python cython problems get resolved by ignoring the cython module. I took a look to see whether this would be a problem for the cython developers, but they themselves don't seem to do cimports.

Problem is that just setting a default for ignored-imports doesn't cut it because it would be overridden by any user configuration.

The option reading code is complex and I don't know where in the codebase it is that you would like to have the forced ignored-import added. I'll do it if you give me a hint.

@Pierre-Sassoulas Pierre-Sassoulas added the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Jan 10, 2023
Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

We don't test with cython right? So this PR doesn't make a lot of sense in its current state imo.

@jtoledo1974
Copy link
Author

We don't test with cython right? So this PR doesn't make a lot of sense in its current state imo.

The point of the matter is that this is not just cython code. It is valid python interpreted code. Cython allows to compile it as well, but it shouldn't be flagged as invalid.

@DanielNoord
Copy link
Collaborator

We don't test with cython right? So this PR doesn't make a lot of sense in its current state imo.

The point of the matter is that this is not just cython code. It is valid python interpreted code. Cython allows to compile it as well, but it shouldn't be flagged as invalid.

I don't really understand your point. I was just saying that this test will never run in CI which is why this PR has a successful CI even though there is no fix for the added test.

@jtoledo1974
Copy link
Author

We don't test with cython right? So this PR doesn't make a lot of sense in its current state imo.

The point of the matter is that this is not just cython code. It is valid python interpreted code. Cython allows to compile it as well, but it shouldn't be flagged as invalid.

I don't really understand your point. I was just saying that this test will never run in CI which is why this PR has a successful CI even though there is no fix for the added test.

I apologize. I misunderstood and my choice of words was poor.

I was forcing the test to run to make sure that it failed first and then it passed. I would say it was run together with the other tests after a full run, but I can't say for sure. But if CI passes I guess it didn't.

@jacobtylerwalls jacobtylerwalls added Waiting on author Indicate that maintainers are waiting for a message of the author and removed Needs review 🔍 Needs to be reviewed by one or multiple more persons labels Jun 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2023

This PR needs take over because because it has been open 8 weeks with no activity.

@github-actions github-actions bot added the Needs take over 🛎️ Orignal implementer went away but the code could be salvaged. label Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs take over 🛎️ Orignal implementer went away but the code could be salvaged. Waiting on author Indicate that maintainers are waiting for a message of the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants