search

hohav / py-slippi

Python library for parsing SSBM replay files
MIT License
56 stars 25 forks source link

Add type annotations #27

Closed dawsonbooth closed 3 years ago

dawsonbooth commented 3 years ago

Thank you for this awesome package!

This PR adds type annotations to all public classes and functions. In essence, each variable that already has a type comment has been given an official, Pythonic type annotation according to PEP 526.

PEP 526 further establishes that "Python will remain a dynamically typed language, and the authors have no desire to ever make type hints mandatory, even by convention." As such, this PR should not affect package's performance or functionality beyond improving the developer experience. That said, I feel I should add that all tests are passing!

I'm relatively new to open source collaboration, so my apologies if something I've done here isn't entirely kosher.

Thanks again for your work on this package! Feel free to let me know of any concerns.

hohav commented 3 years ago

Awesome work! I haven't reviewed the PR in depth yet, but it looks good so far. I haven't used PEP 526 annotations before, so I need to look into a few things before merging.

You don't necessarily have to answer all of the questions I'm about to list, or address them in your PR. I'll investigate them as I have time, but any insight you can offer would be very welcome.

  1. What kind of IDE features does this enable? I don't use an IDE but would still be interested to see how it helps other developers.
  2. Can autodoc docstring comments can be placed on these annotations?
  3. Can the type annotations themselves be added automatically to those docstrings? It would be nice to avoid the redundant Sphinx type annotations (:py:class:`DashBack`:, etc). I vaguely recall looking into this without success back when I started py-slippi, but things may have changed since then. I would also be open to other tools than Sphinx.
  4. Can we auto-generate __slots__ too? Not that important, but it would be a sweet bonus.
  5. Is there a nice way to test whether type annotations match the actual values? Either in a separate test, or as an automatic check that can be enabled for existing tests (like Clojure's spec instrumentation).
dawsonbooth commented 3 years ago

To address your questions:

  1. I use VSCode for most of my Python work, so while I imagine it's useful for all IDE's, I really only understand the benefits therein. The two main features this provides for this IDE is an indication of the parameter types for any given function and auto-complete for the attributes of any given class. Here are a couple of screenshots:

    Screen Shot 2020-12-24 at 12 48 30 PM Screen Shot 2020-12-24 at 3 41 06 PM

  2. I've never used the tool, but this package gives some insight into using PEP 526 annotations with autodoc and Sphinx.

  3. See above

  4. Not sure if there's a true Pythonic way of doing this, but this tool happens to exist for that purpose.

  5. The mypy type checker is an optional static type checker that can run as part of your unit tests or development workflow.

dawsonbooth commented 3 years ago

Just pushed a few commits to address your comments. I believe I made all the changes you requested. I see what you mean about the parse function -- mypy's apparent issues with the union type are explained decently here in their docs.

hohav commented 3 years ago

Thanks for addressing my comments. I merged your PR into this branch for some additional changes; I hope to merge that into master within the week.

Thanks again for the great work!