Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up existing mypy issues, run in CI. #161

Merged
merged 4 commits into from
Jul 9, 2021
Merged

Conversation

DanAlbert
Copy link
Collaborator

This is the first step for #160.

Each commit here was already squashed, so should be independently reviewable (and can rebase the PR when submitting and it won't make a mess of the branch history).

There are four commits here:

  1. Fixes/deletes some files that were just plain broken in the current repo. Not all the vehicle/ship/etc class names were fixed after one of the recent DCS exports. I deleted one since it's presumably unused, but I fixed templates.py since it was used elsewhere. I can resurrect the deleted file if preferred, but it seems no one uses it because it's been broken for a while.
  2. Latest export before mypy changes. I needed to tweak a few things in the exporter to add missing annotations and fix a bug or two, and this commit makes it easier to see the result of those later changes.
  3. The interesting part: cleans up all the mypy errors that exist for the default(ish) configuration. I did punt on the terrain files since that exporter also needs to be fixed, and it's a lot more annoying to run those so I haven't gotten to them yet. For now they're disabled in the mypy.ini, but no other mypy configuration is used beyond the default yet.
  4. Run mypy as part of the GitHub action to prevent regressions.

Most of the fixes needed were pretty straightforward. New base classes, correcting existing annotations, adding missing annotations, introducing a few generics, etc. A very small number of APIs changed, and mostly only to correct the signature to the actual behavior (a handful of functions treat all arguments as optional even though if that behavior is relied upon the function will raise). Shouldn't be any serious impact from the changes.

Many of the function-internal issues were fixed by adding an assertion or a type-check that raises since the existing APIs can't be made type-safe without being more invasive, and IMO that's better left until after mypy is fully deployed. It might also break existing users.

DanAlbert added 2 commits July 8, 2021 18:43
These two files were missed in the latest DCS export. I've deleted the
random mission generator since if no one has complained it must not be
used, but templates.py is used in other places within pydcs so I fixed
that one.
@DanAlbert DanAlbert mentioned this pull request Jul 9, 2021
5 tasks
@DanAlbert DanAlbert force-pushed the mypy branch 3 times, most recently from dda3256 to f24a442 Compare July 9, 2021 02:31
@DanAlbert
Copy link
Collaborator Author

For whatever reason my first attempt at disabling the checking for the terrain files worked on my machine but not in CI. Fixed now and ready to merge as far as I'm concerned. I'll be following up with the other items mentioned in the bug.

DanAlbert added 2 commits July 8, 2021 22:08
Part of pydcs#160.

In many cases this is being solved with assert/raise to inform mypy of
the "guaranteed" types. They could be removed later with some changes to
the implementation/API, but those will be easier to make once the type
checker is fully on.

There are a small number of API changes here, primarily removing default
kwargs that aren't actually defaultable (using the defaults would result
in a crash).
@rp- rp- merged commit 2b4eb8b into pydcs:master Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants