mre / timelapse

🎬 Native macOS app for recording timelapse videos of your desktop.
https://endler.dev/2020/timelapse/
Apache License 2.0
214 stars 18 forks source link

use python 3.6 typing #33

Closed orcutt989 closed 4 years ago

orcutt989 commented 4 years ago

all variables and functions are typed in order to help with refactoring fix #31

Description

Types of Changes

Issues Fixed or Closed by This PR

31

Checklist

mre commented 4 years ago

Amazing! That was fast. 🚀 Looks like flake8 doesn't like our type hints. Maybe there's an easy fix?

orcutt989 commented 4 years ago

Thoughts @mre https://stackoverflow.com/questions/43187829/is-there-a-way-to-make-flake8-check-for-type-hints-in-the-source?

mre commented 4 years ago

Skipped some more type hints for reassignments. Type hints are only allowed on when a new variable gets introduced. 😉 Think this PR is ready to get merged! 🎉 Thanks for pushing this forward @orcutt989. timelapse has types thanks to you!

orcutt989 commented 4 years ago

Wooo! Thanks for letting me help!

estysdesu commented 4 years ago

Does it make sense at this point to add mypy to the dev-dependencies now for type checking? Or is there some other way these are being checked?

Mypy would greatly help catch things like how a float type was passed to a function argument that is typed as int. It probably should be typed as Union[int, float] or something numeric like such. Although not big for this case as Python can handle that without hitting an exception, mypy also helped detect when passing an Exception type to an argument thats expecting a str which could be a bigger issue (not in this case though).

EDIT: mypy reports multiple typing errors (omitting the library stubs missing as that was expected and can be configured off):

(timelapse-aCV8DQCs-py3.7) bash-5.0$ mv timelapse/__init.py__ timelapse/__init__.py    # see issue #43 
(timelapse-aCV8DQCs-py3.7) bash-5.0$ mypy -p timelapse
timelapse/__main__.py:9: error: Cannot find implementation or library stub for module named 'notify'    # imports should be renamed for packages to import timelapse.notify or from . import notify (https://docs.python.org/3.7/tutorial/modules.html#intra-package-references)
timelapse/__main__.py:10: error: Cannot find implementation or library stub for module named 'encoder'    # see above
timelapse/__main__.py:11: error: Cannot find implementation or library stub for module named 'recorder'    # see above
timelapse/__main__.py:47: error: Name 'NSObject' is not defined    # to satisfy mypy, all of these objects should be explicitly imported (from AppKit import NSObject, NSStatusBar, ...) or use # type: ignore
timelapse/__main__.py:63: error: Name 'NSStatusBar' is not defined
...
timelapse/__main__.py:99: error: Name 'NSMenuItem' is not defined
timelapse/__main__.py:103: error: Name 'NSMenuItem' is not defined
timelapse/__main__.py:110: error: "None" has no attribute "join"    # typing is not present for self.recorder; mypy infered incorrect type because it was set to None; type should be Optional[Recorder]
timelapse/__main__.py:121: error: Attribute 'recording' already defined on line 54    # type defined twice
Found 24 errors in 3 files (checked 5 source files)

I'd be happy to submit a PR that

  1. Adds mypy to the dev dependencies
  2. Cleans up mypy issues

Just let me know if this is something you'd be interested in. Glad to see this project make use of Python typing. I think its really valuable.

mre commented 4 years ago

Thanks for looking into this @estysdesu. A PR is most welcome. As a start, I've added a Python linter to the CI build process in #45, but we should still add mypy to the dev dependencies and clean up the issues. 👍

mre commented 4 years ago
Your code has been rated at 1.47/10

There's a bit of work to do, I guess. 😆