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

Modify python version check such that linters recognize it #2039

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

omer54463
Copy link
Contributor

This is sort of a follow up to #2005. Currently many type checkers have a hard time recognizing all the different ways you can compare versions with sys.version_info (it's a best-effort kind of thing).

I changed the comparison and made sure that:

  1. It works for both python 3 and python 2.7.
  2. Mypy and Pyright both understand the comparison and skip the python 2.7 implementation.

@elicn
Copy link
Contributor

elicn commented Oct 19, 2024

Personally I like less the half-tuple comparison (i.e. (3, )).
What wrong with the current method? I don't see any problems with MyPy or Pylint on my side.

@omer54463
Copy link
Contributor Author

omer54463 commented Oct 23, 2024

Here's what I'm experiencing:

uc = unicorn.Uc(unicorn.UC_ARCH_X86, unicorn.UC_MODE_64)
reveal_type(uc) # Revealed type is "unicorn.unicorn_py2.Uc"mypy(note)

Which aligns with what Mypy claims:

Mypy supports the ability to perform Python version checks and platform checks (e.g. Windows vs Posix), ignoring code paths that won’t be run on the targeted Python version or platform. This allows you to more effectively typecheck code that supports multiple versions of Python or multiple operating systems.

More specifically, mypy will understand the use of sys.version_info and sys.platform checks within if/elif/else statements. For example:

import sys

# Distinguishing between different versions of Python:
if sys.version_info >= (3, 8):
    # Python 3.8+ specific definitions and imports
else:
    # Other definitions and imports

# Distinguishing between different operating systems:
if sys.platform.startswith("linux"):
    # Linux-specific code
elif sys.platform == "darwin":
    # Mac-specific code
elif sys.platform == "win32":
    # Windows-specific code
else:
    # Other systems

...

Mypy currently does not support more complex checks, and does not assign any special meaning when assigning a sys.version_info or sys.platform check to a variable. This may change in future versions of mypy.

if _sys.version_info.major == 2:
from .unicorn_py2 import *
else:
if _sys.version_info >= (3,):
Copy link
Contributor

Choose a reason for hiding this comment

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

It could make sense if we were differentiating between different minor versions, but only comparing for major version this is less readable.

@@ -23,8 +23,7 @@
if not hasattr(sys.modules[__name__], "__file__"):
__file__ = inspect.getfile(inspect.currentframe())

_python2 = sys.version_info[0] < 3
if _python2:
if sys.version_info < (3,):
Copy link
Contributor

Choose a reason for hiding this comment

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

This piece of code is redundant altogether. Python 3 never gets here anyway.

@@ -9,7 +9,7 @@
from unicorn.x86_const import *


if sys.version_info.major == 2:
if sys.version_info < (3,):
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned above, this is less readable when comparing major version only.

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

Successfully merging this pull request may close these issues.

2 participants