Add type checking to entire versioneer project (excluding tests) by Callek · Pull Request #367 · python-versioneer/python-versioneer

Callek · 2023-04-22T03:56:13Z

This adds enough of the type checking, (and fixes a few minor type issues) for the entirety of the versioneer repository.

There are still better type checking things we could do, but this is a great start and should allow at least some type check abilities of dependent projects

Using 'Any' for the CommandClass variables bypasses some complications with the type system, for example setuptools and distutils Command classes are different, as well as the variable gets complicated with when used as a base class because its assigning an object not a 'Type' per mypy scanning.

effigies · 2023-04-23T12:43:39Z

setup.py

-class develop(develop):
-    def run(self):
+class develop(_develop):
+    def run(self) -> None:  # type: ignore[override]


What was the original return type? Or was it marked final?

So this one was a bit confusing at first.

The upstream types-setuptools defines it like this: https://github.com/python/typeshed/blob/acfde4e40b3a75aeff1d6324a4eb9adbca5579e2/stubs/setuptools/setuptools/command/develop.pyi#L12 (basically using the # type: ignore[override] as well. Because its the same method definition.

They need this in that upstream because the setuptool develop command depends on its easy_install command, which added a flag for the "Deprecation" warning it uses: https://github.com/python/typeshed/blob/acfde4e40b3a75aeff1d6324a4eb9adbca5579e2/stubs/setuptools/setuptools/command/easy_install.pyi#L54

So because the method definitions differ, we need the ignore[override] here. Nothing actually breaks in implementation though!

I wonder if it makes more sense to just subclass from Command, since we don't actually do anything but raise an exception. Then no need to ignore.

effigies · 2023-04-23T12:45:50Z

src/cmdclass.py



-def get_cmdclass(cmdclass=None):
+def get_cmdclass(cmdclass: Optional[Dict[str, Any]] = None):


Shouldn't this be:

Suggested change

def get_cmdclass(cmdclass: Optional[Dict[str, Any]] = None):

def get_cmdclass(cmdclass: Optional[Dict[str, Type[Command]]] = None):

effigies · 2023-04-23T12:46:07Z

src/cmdclass.py

    # we override different "build_py" commands for both environments
    if 'build_py' in cmds:
-        _build_py = cmds['build_py']
+        _build_py: Any = cmds['build_py']


Suggested change

_build_py: Any = cmds['build_py']

_build_py: Type[Command] = cmds['build_py']

... and so on.

I did this (and the other Any stuff in this file) due to errors like the following that were harder to detangle:

versioneer.py:1934: error: Incompatible import of "_build_py" (imported name has type "Type[build_py]", local name has type "Type[Command]") [assignment] versioneer.py:1936: error: Variable "_build_py" is not valid as a type [valid-type] versioneer.py:1936: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases versioneer.py:1936: error: Invalid base class "_build_py" [misc]

From looking things up I might have been able to solve it if we required typing_extensions installed to use versioneer, but I didn't want to add that as a requirement here - at least right now.

I may be able to solve this using TypeVar and similar logic, but I decided that was best tackled separately.

Fair enough. Any marks an opportunity for refinement, so it won't be hard to spot.

effigies · 2023-04-23T12:59:48Z

src/header.py



-def register_vcs_handler(vcs, method):  # decorator
+def register_vcs_handler(vcs: str, method: str) -> Callable:  # decorator


Not sure if it would actually capture any bad calls, but we can use a TypeVar to let the checker know that it's a decorator that doesn't change the type of the decorated object:

Suggested change

def register_vcs_handler(vcs: str, method: str) -> Callable: # decorator

F = TypeVar('F', bound=Callable)

def register_vcs_handler(vcs: str, method: str) -> Callable[[F], F]: # decorator

At least, I think this should work. Haven't tried it.

I agree this would be a lot better than what I have here, however I'm considering doing even better than this in a future patch. I didn't want to tie up this larger improvement with further improving this decorator (mainly since users of the library will be unlikely to ever need to touch this decorator.)

That said, if you feel strongly I can do that extra improvement as part of this PR.

I don't feel strongly.

effigies · 2023-04-23T12:59:58Z

src/header.py

+def register_vcs_handler(vcs: str, method: str) -> Callable:  # decorator
    """Create decorator to mark a method as the handler of a VCS."""
-    def decorate(f):
+    def decorate(f: Callable) -> Callable:


Suggested change

def decorate(f: Callable) -> Callable:

def decorate(f: F) -> F:

See comment at #367 (comment)

Callek · 2023-05-01T19:42:13Z

@effigies - Are we good to merge given our discussion, or is there something more I should do here?

(also, do you want me to continue to hold off on merging until you've approved going foward? - I see I have the button but I generally like to merge after review when it makes sense)

effigies · 2023-05-01T19:44:37Z

Ah, sorry. For some reason I thought this was in your court. I'm okay with this as-is. Merging.

Callek added 9 commits April 19, 2023 23:47

Add typing of versioneer.py to tox

810ac5d

Add type information to 'header.py'

ebd29f1

Add type information to 'git/install.py'

2d669f3

Add type information to 'from_file.py'

4fbc7b2

Add type information to 'get_versions.py'

6cfb3a2

Add type information to 'setupfunc.py'

f6ed358

Add type information to 'installer.py'

c3495b7

Type check the root setup.py for versioneer

754e634

Callek requested a review from effigies April 22, 2023 03:56

Callek self-assigned this Apr 22, 2023

effigies reviewed Apr 23, 2023

View reviewed changes

Callek requested a review from effigies April 24, 2023 00:35

effigies merged commit 8493ad0 into python-versioneer:master May 1, 2023



		def get_cmdclass(cmdclass=None):
		def get_cmdclass(cmdclass: Optional[Dict[str, Any]] = None):

	_build_py: Any = cmds['build_py']
	_build_py: Type[Command] = cmds['build_py']



		def register_vcs_handler(vcs, method): # decorator
		def register_vcs_handler(vcs: str, method: str) -> Callable: # decorator

-def register_vcs_handler(vcs: str, method: str) -> Callable:  # decorator
+F = TypeVar('F', bound=Callable)
+def register_vcs_handler(vcs: str, method: str) -> Callable[[F], F]:  # decorator

	def decorate(f: Callable) -> Callable:
	def decorate(f: F) -> F:

Conversation

Callek commented Apr 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Callek commented May 1, 2023

Uh oh!

effigies commented May 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants