search

matthew-brett / delocate

Find and copy needed dynamic libraries into python wheels
BSD 2-Clause "Simplified" License
262 stars 59 forks source link

Add Python 2 style type-hinting. Enforce Flake8 and MyPy linters with GitHub Actions. #93

Closed HexDecimal closed 2 years ago

HexDecimal commented 3 years ago

Everything in the delocate directory with the exception of _version.py has been type annotated.

Code has been converted to use Unicode more often, this is enforced better in Python 2 via type hinting. Six is used in some places to ensure compatibility with Python 2. This can be implemented in the package itself if you don't like six for some reason.

back_tick was too polymorphic to gracefully type hint so it's been split into two calls. check_call which is like back_tick's default behavior and run_call which returns the error output instead of raising an exception. In addition both functions take and return Unicode strings exclusively. Tests and code have been updated with the new functions. This is the best I can do without dropping Python 2.7 or adding subprocess32.

InWheelCtx violated its supertype by overriding __enter__ with a different return type. I've changed this class into a function with similar behavior. InWheelCtx tests have not been changed. MyPy doesn't like the tests but the current code will work well in practice. This new function can be turned into a method of InWheel if you really want support for class inheritance. InWheelCtx can also be a class that encapsulates InWheel, it just can't sub-class it.

The type hinting gives a better picture of how things are structured. The common lib_dict type is Dict[Text, Dict[Text, Text]] and you can tell it's used pretty often. If we wanted to refactor that type it can now be done pretty easily.

py.typed is PEP 561 and marks the package as supporting type hinting. The GitHub actions support their own kind of annotations which should show errors in commits and pull requests. The MyPy configuration is only set up to detect critical issues rather than most issues.

matthew-brett commented 3 years ago

Thanks a lot for this - it is useful.

For your questions / points for discussion.

I would like to continue to have an implementation back_tick, for back-compatibility, but it is fine to deprecate it in favor of your refactored functions. Can we just turn off type checking for that deprecated function?

For InWheelCtx - I'd rather leave that as a class, but it's fine to encapsulate rather than inherit.

HexDecimal commented 3 years ago

back_tick is now as it was before, but has now been deprecated. I've guessed a deprecated version number to document which might need to be changed. Normally I'd suppress the deprecation warnings using pytest, but I'm not sure what the pytest_tools module is doing exactly.

InWheelCtx is now a class which encapsulates InWheel. It implements its previous behavior using properties. This is the most backward compatible option and works well.

The ret_self parameter has been removed from InWheel. This already did nothing before this PR and it never had any associated tests. This change breaks MyPy's implicit union in the tests for some reason, but that was a minor issue.

HexDecimal commented 3 years ago

I think this PR is supposed to pass tests, but the jobs need to be reset manually. I can see the same issue with some other failing PR's on here.

HexDecimal commented 3 years ago

Splitting type hinting from modifications makes sense, but putting each file in its own pull request seems overkill.

I can make a PR where only the trivial hints are added. Everything after that is to fix type errors revealed by type hinting.

matthew-brett commented 3 years ago

@rowillia - great - thank you! @HexDecimal - your last suggestion about splitting up the PR seems very reasonable.

HexDecimal commented 3 years ago

@rowillia was correct on how to do things. In practice it's easier to add type-hints in per-file batches. I may have burned myself out before realizing this.

Here's what adding the type hints without also adding the fixes looks like:

$ mypy delocate/
delocate\tools.py:87: error: Incompatible types in assignment (expression has type "unicode", variable has type "str")
delocate\tools.py:92: error: Incompatible types in assignment (expression has type "unicode", variable has type "str")
delocate\tools.py:182: error: Item "None" of "Optional[Match[unicode]]" has no attribute "groups"
delocate\tools.py:182: error: Incompatible return value type (got "Union[Tuple[unicode, ...], Any]", expected "Tuple[unicode, unicode, unicode]")
delocate\tools.py:364: error: Item "None" of "Optional[Match[unicode]]" has no attribute "groups"
delocate\tools.py:545: error: Incompatible types in assignment (expression has type "Pattern[str]", variable has type "str")
delocate\tools.py:546: error: "str" has no attribute "match"
delocate\tests\scriptrunner.py:107: error: No overload variant of "get" of "Mapping" matches argument types "unicode", "bool"
delocate\tests\scriptrunner.py:107: note: Possible overload variant:
delocate\tests\scriptrunner.py:107: note:     def [_T] get(self, k: str, default: Union[str, _T]) -> Union[str, _T]
delocate\tests\scriptrunner.py:107: note:     <1 more non-matching overload not shown>
delocate\tests\scriptrunner.py:158: error: Incompatible types in assignment (expression has type "Dict[str, str]", variable has type "_Environ[str]")
delocate\tests\scriptrunner.py:161: error: Incompatible types in assignment (expression has type "unicode", target has type "str")
delocate\tests\scriptrunner.py:163: error: Incompatible types in assignment (expression has type "unicode", target has type "str")
delocate\wheeltools.py:160: error: Return type "InWheelCtx" of "__enter__" incompatible with return type "unicode" in supertype "InWheel"
delocate\wheeltools.py:160: error: Return type "InWheelCtx" of "__enter__" incompatible with return type "unicode" in supertype "InTemporaryDirectory"
delocate\wheeltools.py:160: error: Return type "InWheelCtx" of "__enter__" incompatible with return type "unicode" in supertype "TemporaryDirectory"
delocate\libsana.py:287: error: No overload variant of "get" of "Mapping" matches argument type "unicode"
delocate\libsana.py:287: note: Possible overload variant:
delocate\libsana.py:287: note:     def get(self, k: str) -> Optional[str]
delocate\libsana.py:287: note:     <1 more non-matching overload not shown>
delocate\tests\env_tools.py:16: error: No overload variant of "get" of "Mapping" matches argument types "unicode", "None"
delocate\tests\env_tools.py:16: note: Possible overload variant:
delocate\tests\env_tools.py:16: note:     def [_T] get(self, k: str, default: Union[str, _T]) -> Union[str, _T]
delocate\tests\env_tools.py:16: note:     <1 more non-matching overload not shown>
delocate\tests\env_tools.py:18: error: Argument 1 to "__delitem__" of "_Environ" has incompatible type "unicode"; expected "str"
delocate\tests\env_tools.py:25: error: Invalid index type "unicode" for "_Environ[str]"; expected type "str"
delocate\tests\env_tools.py:28: error: Argument 1 to "__delitem__" of "_Environ" has incompatible type "unicode"; expected "str"
delocate\delocating.py:308: error: Argument 2 to "tree_libs" has incompatible type "Union[str, Callable[[unicode], bool], None]"; expected "Optional[Callable[[unicode], bool]]"
delocate\delocating.py:575: error: Need more than 2 values to unpack (3 expected)
delocate\delocating.py:582: error: Too many values to unpack (2 expected, 3 provided)
delocate\tests\test_wheeltools.py:89: error: Value of type variable "AnyStr" of "realpath" cannot be "Optional[unicode]"
delocate\tests\test_wheeltools.py:156: error: Value of type variable "AnyStr" of "realpath" cannot be "Optional[unicode]"
delocate\tests\test_wheeltools.py:174: error: Value of type variable "AnyStr" of "realpath" cannot be "Optional[unicode]"
delocate\tests\test_wheeltools.py:178: error: Value of type variable "AnyStr" of "realpath" cannot be "Optional[unicode]"
delocate\tests\test_libsana.py:32: error: Incompatible return value type (got "Dict[str, Dict[unicode, str]]", expected "Dict[unicode, Dict[unicode, unicode]]")
delocate\tests\test_libsana.py:137: error: Incompatible return value type (got "Dict[str, Dict[unicode, str]]", expected "Dict[unicode, Dict[unicode, unicode]]")
delocate\tests\test_delocating.py:423: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]"
delocate\tests\test_delocating.py:424: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]"
delocate\tests\test_delocating.py:430: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]"
delocate\tests\test_delocating.py:432: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]"
delocate\tests\test_delocating.py:433: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]"
delocate\tests\test_delocating.py:434: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]"
delocate\tests\test_delocating.py:435: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]"
delocate\tests\test_delocating.py:436: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]"
delocate\tests\test_delocating.py:448: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]"
delocate\tests\test_delocating.py:451: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]"
delocate\tests\test_delocating.py:453: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]"
delocate\tests\test_delocating.py:455: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]"
delocate\tests\test_delocating.py:481: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]"
delocate\tests\test_delocating.py:483: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]"
delocate\tests\test_delocating.py:573: error: Incompatible types in assignment (expression has type "unicode", target has type "str")
delocate\tests\test_scripts.py:272: error: Incompatible types in assignment (expression has type "str", variable has type "List[unicode]")
delocate\tests\test_scripts.py:276: error: "List[unicode]" has no attribute "decode"
delocate\tests\test_scripts.py:277: error: "List[unicode]" has no attribute "startswith"
delocate\tests\test_scripts.py:278: error: "List[unicode]" has no attribute "endswith"
delocate\tests\test_scripts.py:280: error: "List[unicode]" has no attribute "strip"
delocate\tests\test_scripts.py:283: error: Incompatible types in assignment (expression has type "str", variable has type "List[unicode]")
delocate\tests\test_scripts.py:287: error: "List[unicode]" has no attribute "decode"
delocate\tests\test_scripts.py:288: error: "List[unicode]" has no attribute "startswith"
delocate\tests\test_scripts.py:289: error: "List[unicode]" has no attribute "endswith"
delocate\tests\test_scripts.py:291: error: "List[unicode]" has no attribute "decode"
delocate\tests\test_scripts.py:381: error: Unsupported operand types for + ("List[unicode]" and "List[str]")
delocate\tests\test_scripts.py:385: error: Unsupported operand types for + ("List[unicode]" and "List[str]")
delocate\tests\test_scripts.py:392: error: Unsupported operand types for + ("List[unicode]" and "List[str]")
delocate\tests\test_scripts.py:404: error: Unsupported operand types for + ("List[unicode]" and "List[str]")
delocate\tests\test_scripts.py:407: error: Unsupported operand types for + ("List[unicode]" and "List[str]")
delocate\tests\test_scripts.py:425: error: Unsupported operand types for + ("List[unicode]" and "List[str]")
delocate\tests\test_scripts.py:433: error: Unsupported operand types for + ("List[unicode]" and "List[str]")
delocate\tests\test_scripts.py:438: error: Unsupported operand types for + ("List[unicode]" and "List[str]")
delocate\tests\test_scripts.py:447: error: Unsupported operand types for + ("List[unicode]" and "List[str]")
delocate\tests\test_scripts.py:452: error: Unsupported operand types for + ("List[unicode]" and "List[str]")
delocate\tests\test_scripts.py:459: error: Unsupported operand types for + ("List[unicode]" and "List[str]")
Found 64 errors in 10 files (checked 30 source files)

Half of these are Python 2 str/unicode issues. If Python 2 was dropped then these would go away without needing to change anything else. os.environ is really bad here since it can't accept Unicode in Python 2. These can be fixed in Python 2 by using six's string functions and by adding the u prefix to most strings.

Regular expressions are used a few times without checking their return values for None.

Some of these are just issues with MyPy. It has a hard time sorting tuples by length. It also doesn't allow names to switch types in the middle of a function.

I've fixed these already, but it's gotten very tedious to reorganize my commits to make them easier to review.

matthew-brett commented 3 years ago

Would it help if we made the next release Python 3 only?

On Thu, Feb 25, 2021 at 3:27 PM Kyle Benesch notifications@github.com wrote:

@rowillia https://github.com/rowillia was correct on how to do things. In practice it's easier to add type-hints in per-file batches. I may have burned myself out before realizing this.

Here's what adding the type hints without also adding the fixes looks like:

$ mypy delocate/ delocate\tools.py:87: error: Incompatible types in assignment (expression has type "unicode", variable has type "str") delocate\tools.py:92: error: Incompatible types in assignment (expression has type "unicode", variable has type "str") delocate\tools.py:182: error: Item "None" of "Optional[Match[unicode]]" has no attribute "groups" delocate\tools.py:182: error: Incompatible return value type (got "Union[Tuple[unicode, ...], Any]", expected "Tuple[unicode, unicode, unicode]") delocate\tools.py:364: error: Item "None" of "Optional[Match[unicode]]" has no attribute "groups" delocate\tools.py:545: error: Incompatible types in assignment (expression has type "Pattern[str]", variable has type "str") delocate\tools.py:546: error: "str" has no attribute "match" delocate\tests\scriptrunner.py:107: error: No overload variant of "get" of "Mapping" matches argument types "unicode", "bool" delocate\tests\scriptrunner.py:107: note: Possible overload variant: delocate\tests\scriptrunner.py:107: note: def [_T] get(self, k: str, default: Union[str, _T]) -> Union[str, _T] delocate\tests\scriptrunner.py:107: note: <1 more non-matching overload not shown> delocate\tests\scriptrunner.py:158: error: Incompatible types in assignment (expression has type "Dict[str, str]", variable has type "_Environ[str]") delocate\tests\scriptrunner.py:161: error: Incompatible types in assignment (expression has type "unicode", target has type "str") delocate\tests\scriptrunner.py:163: error: Incompatible types in assignment (expression has type "unicode", target has type "str") delocate\wheeltools.py:160: error: Return type "InWheelCtx" of "enter" incompatible with return type "unicode" in supertype "InWheel" delocate\wheeltools.py:160: error: Return type "InWheelCtx" of "enter" incompatible with return type "unicode" in supertype "InTemporaryDirectory" delocate\wheeltools.py:160: error: Return type "InWheelCtx" of "enter" incompatible with return type "unicode" in supertype "TemporaryDirectory" delocate\libsana.py:287: error: No overload variant of "get" of "Mapping" matches argument type "unicode" delocate\libsana.py:287: note: Possible overload variant: delocate\libsana.py:287: note: def get(self, k: str) -> Optional[str] delocate\libsana.py:287: note: <1 more non-matching overload not shown> delocate\tests\env_tools.py:16: error: No overload variant of "get" of "Mapping" matches argument types "unicode", "None" delocate\tests\env_tools.py:16: note: Possible overload variant: delocate\tests\env_tools.py:16: note: def [_T] get(self, k: str, default: Union[str, _T]) -> Union[str, _T] delocate\tests\env_tools.py:16: note: <1 more non-matching overload not shown> delocate\tests\env_tools.py:18: error: Argument 1 to "delitem" of "_Environ" has incompatible type "unicode"; expected "str" delocate\tests\env_tools.py:25: error: Invalid index type "unicode" for "_Environ[str]"; expected type "str" delocate\tests\env_tools.py:28: error: Argument 1 to "delitem" of "_Environ" has incompatible type "unicode"; expected "str" delocate\delocating.py:308: error: Argument 2 to "tree_libs" has incompatible type "Union[str, Callable[[unicode], bool], None]"; expected "Optional[Callable[[unicode], bool]]" delocate\delocating.py:575: error: Need more than 2 values to unpack (3 expected) delocate\delocating.py:582: error: Too many values to unpack (2 expected, 3 provided) delocate\tests\test_wheeltools.py:89: error: Value of type variable "AnyStr" of "realpath" cannot be "Optional[unicode]" delocate\tests\test_wheeltools.py:156: error: Value of type variable "AnyStr" of "realpath" cannot be "Optional[unicode]" delocate\tests\test_wheeltools.py:174: error: Value of type variable "AnyStr" of "realpath" cannot be "Optional[unicode]" delocate\tests\test_wheeltools.py:178: error: Value of type variable "AnyStr" of "realpath" cannot be "Optional[unicode]" delocate\tests\test_libsana.py:32: error: Incompatible return value type (got "Dict[str, Dict[unicode, str]]", expected "Dict[unicode, Dict[unicode, unicode]]") delocate\tests\test_libsana.py:137: error: Incompatible return value type (got "Dict[str, Dict[unicode, str]]", expected "Dict[unicode, Dict[unicode, unicode]]") delocate\tests\test_delocating.py:423: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]" delocate\tests\test_delocating.py:424: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]" delocate\tests\test_delocating.py:430: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]" delocate\tests\test_delocating.py:432: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]" delocate\tests\test_delocating.py:433: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]" delocate\tests\test_delocating.py:434: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]" delocate\tests\test_delocating.py:435: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]" delocate\tests\test_delocating.py:436: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]" delocate\tests\test_delocating.py:448: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]" delocate\tests\test_delocating.py:451: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]" delocate\tests\test_delocating.py:453: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]" delocate\tests\test_delocating.py:455: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]" delocate\tests\test_delocating.py:481: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]" delocate\tests\test_delocating.py:483: error: Argument 1 to "check_archs" has incompatible type "Dict[str, Dict[str, str]]"; expected "Dict[unicode, Dict[unicode, unicode]]" delocate\tests\test_delocating.py:573: error: Incompatible types in assignment (expression has type "unicode", target has type "str") delocate\tests\test_scripts.py:272: error: Incompatible types in assignment (expression has type "str", variable has type "List[unicode]") delocate\tests\test_scripts.py:276: error: "List[unicode]" has no attribute "decode" delocate\tests\test_scripts.py:277: error: "List[unicode]" has no attribute "startswith" delocate\tests\test_scripts.py:278: error: "List[unicode]" has no attribute "endswith" delocate\tests\test_scripts.py:280: error: "List[unicode]" has no attribute "strip" delocate\tests\test_scripts.py:283: error: Incompatible types in assignment (expression has type "str", variable has type "List[unicode]") delocate\tests\test_scripts.py:287: error: "List[unicode]" has no attribute "decode" delocate\tests\test_scripts.py:288: error: "List[unicode]" has no attribute "startswith" delocate\tests\test_scripts.py:289: error: "List[unicode]" has no attribute "endswith" delocate\tests\test_scripts.py:291: error: "List[unicode]" has no attribute "decode" delocate\tests\test_scripts.py:381: error: Unsupported operand types for + ("List[unicode]" and "List[str]") delocate\tests\test_scripts.py:385: error: Unsupported operand types for + ("List[unicode]" and "List[str]") delocate\tests\test_scripts.py:392: error: Unsupported operand types for + ("List[unicode]" and "List[str]") delocate\tests\test_scripts.py:404: error: Unsupported operand types for + ("List[unicode]" and "List[str]") delocate\tests\test_scripts.py:407: error: Unsupported operand types for + ("List[unicode]" and "List[str]") delocate\tests\test_scripts.py:425: error: Unsupported operand types for + ("List[unicode]" and "List[str]") delocate\tests\test_scripts.py:433: error: Unsupported operand types for + ("List[unicode]" and "List[str]") delocate\tests\test_scripts.py:438: error: Unsupported operand types for + ("List[unicode]" and "List[str]") delocate\tests\test_scripts.py:447: error: Unsupported operand types for + ("List[unicode]" and "List[str]") delocate\tests\test_scripts.py:452: error: Unsupported operand types for + ("List[unicode]" and "List[str]") delocate\tests\test_scripts.py:459: error: Unsupported operand types for + ("List[unicode]" and "List[str]") Found 64 errors in 10 files (checked 30 source files)

Half of these are Python 2 str/unicode issues. If Python 2 was dropped then these would go away without needing to change anything else. os.environ is really bad here since it can't accept Unicode in Python 2. These can be fixed in Python 2 by using six's string functions and by adding the u prefix to most strings.

Regular expressions are used a few times without checking their return values for None.

Some of these are just issues with MyPy. It has a hard time sorting tuples by length. It also doesn't allow names to switch types in the middle of a function.

I've fixed these already, but it's gotten very tedious to reorganize my commits to make them easier to review.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/matthew-brett/delocate/pull/93#issuecomment-785985308, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAQQHEUOESU5N5GX6XTSFLTAZT63ANCNFSM4VSA2TKA .

HexDecimal commented 3 years ago

That would help a lot. Python 2.7 code has gotten really hard to maintain.

matthew-brett commented 3 years ago

I'm happy to go to Python 3, for the next release - and therefore, for the current code-base. Any interest in getting that going?