frerich / clcache

A compiler cache for MSVC, much like ccache for gcc
Other
324 stars 83 forks source link

Made clcache source code pass mypy checks #293

Closed frerich closed 7 years ago

frerich commented 7 years ago

There were four issues which made 'mypy' complain about the clcache source code:

  1. There was a syntax error in a type comment; mypy complained:

    clcache.py:1254: error: syntax error in type comment

It appears that for function types, the arguments need to be enclosed in parentheses.

  1. mypy failed to process type comments because of various meta types missing; import the relevant types from the 'typing' module, but only if it's available (it was introduced with Python 3.5)

  2. Decorated properties such as

    @property @contextlib.contextmanager def f():

are not supported by mypy and generate an error. Some Google-query reveals that this can be avoided by putting '# type: ignore' on the '@property' line.

  1. mypy seems to generate a .mypy_cache directory - let's have Git ignore that.

With all this in place running

mypy --ignore-missing-imports clcache.py

passes without any errors.

frerich commented 7 years ago

@xoviat Since you seem to be familiar with type annotations in Python in general, and mypy in particular - mind having a look at this? I basically tried mypy clcache.py and tried to resolve the issues found in sensible ways.

ghost commented 7 years ago

@frerich I would highly recommend fixing the setup.py issues before working on this. It will make this so much simpler. I will submit a PR for that.

frerich commented 7 years ago

How would using setup.py make this work "so much simpler"? Doesn't the current clcache Python code need fixing either way to avoid the syntax errors (e.g. the syntax error in the type comment or the lack of List/Any/Iterable etc.) anyway?

ghost commented 7 years ago

How would using setup.py make this work "so much simpler"?

setup.py installs the typing dependency on older Python versions so you don't have to work around the problem with various hacks.

frerich commented 7 years ago

Ah, that's interesting. So a setup.py would remove the need for

+try:
+    from typing import Any, Iterator, List, Tuple
+except ImportError:
+    pass

...right? Are there other benefits? What about the other parts of the patch in this PR, would they also become obsolete?

ghost commented 7 years ago

What about the other parts of the patch in this PR, would they also become obsolete?

Yes, because you would use the inline type annotations rather than the comments. The comments were simply a workaround (incidentally, implementing mypy would break support for installation without PyInstaller/setup.py on Python 3.3 and 3.4, but you seemed okay with dropping them).

frerich commented 7 years ago

Yes, because you would use the inline type annotations rather than the comments.

Wait, what? How does the change to setup.py enable using the inline type annotations? I'm confused. setup.py seemed to be a mere implementation detail specific to packaging Python software via pip, but type annotations are a feature of the Python language (I suppose they are only available in Python 3.5+)?

implementing mypy would break support for installation without PyInstaller/setup.py on Python 3.3 and 3.4

I don't understand what you mean by 'implementing mypy' -- my understanding is that it's simply a linter-style program which we could integrate into the CI system.

ghost commented 7 years ago

Let me show you with a PR

frerich commented 7 years ago

Closing this, superseded by #302.