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 an __all__ variable to constants.c #1733

Merged
merged 8 commits into from May 23, 2020

Conversation

MyreMylar
Copy link
Contributor

An experiment to see if this will fix the LGTM errors.

@lgtm-com
Copy link

lgtm-com bot commented May 14, 2020

This pull request fixes 1 alert when merging 6087114 into 6df1c80 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 14, 2020

This pull request fixes 1 alert when merging 7242bb6 into 6df1c80 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@MyreMylar MyreMylar marked this pull request as ready for review May 14, 2020 20:30
@MyreMylar MyreMylar added Code quality/robustness Code quality and resilience to changes Linters Related to code linting issues or solutions labels May 15, 2020
@illume
Copy link
Member

illume commented May 17, 2020

What was the result of your testing? Does this help any tool?

  • LGTM
  • pylint
  • python help(pygame), help(pygame.locals)
  • jedi
  • pycharm

I expect that some analyze the python code statically, so that the __all__ would need to also be static, not dynamic?

@MyreMylar
Copy link
Contributor Author

MyreMylar commented May 17, 2020

What was the result of your testing? Does this help any tool?

  • LGTM
  • pylint
  • python help(pygame), help(pygame.locals)
  • jedi
  • pycharm

I expect that some analyze the python code statically, so that the __all__ would need to also be static, not dynamic?

From my testing it seems that pylint can pick up the __all__ variable the way I have defined it here. Most of the alerts and errors we got previously have been covered by the .pyi files in pylint, jedi & PyCharm.

using 2.0.0.dev8 if you do:

pylint --extension-pkg-whitelist=pygame --allow-wildcard-with-all y test.py

on a file called test.py containing:

"""
Doc
"""
from pygame.constants import *
print(K_END)

you will get:

test.py:4:0: W0401: Wildcard import pygame.constants (wildcard-import)

But with this pull request compiled you won't. allow-wildcard-with-all is disabled by default though so I guess the more reliable way to fix pylint would be for us to never use wildcard imports in pygame and instead import each constant individually into pygame's __init__.py and locals.py. The downside with that is that it is tedious and basically duplicates the long list of constants in two more places in the code (at least)

LGTM is a mystery to me.

@MyreMylar
Copy link
Contributor Author

MyreMylar commented May 17, 2020

On LGTM,

I don't know how it is checking for the __all__ variable for its alerts, or if the incremental check we get here on pull requests works the same as a full scan (which seem to happen on a slower schedule in my experience). It's possible the version of __all__ I generate in the c file here is not what the LGTM checker is expecting, but without knowing how it checks it's hard to know. I could try emailing the LGTM team at GitHub?

I suspect, as with pylint, the full-proof way to eliminate these alerts is to directly import each constant individually.

@MyreMylar
Copy link
Contributor Author

I'll guess I'll try emailing semmle (creators of LGTM) about this just to see what they say, unless anyone has a better idea?

@MyreMylar
Copy link
Contributor Author

MyreMylar commented May 18, 2020

I have now tried sending an email, will see what response I get.

Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

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

👍 Thanks

@illume illume merged commit 26b1139 into pygame:master May 23, 2020
@MyreMylar MyreMylar deleted the add-all-to-constants branch June 5, 2020 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code quality/robustness Code quality and resilience to changes Linters Related to code linting issues or solutions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants