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.

Sign up for GitHub

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

Jump to bottom

enforced typing on all APIs? #160

Open
3 of 5 tasks
DanAlbert opened this issue Jul 8, 2021 · 2 comments
Open
3 of 5 tasks

enforced typing on all APIs? #160

DanAlbert opened this issue Jul 8, 2021 · 2 comments

Comments

@DanAlbert
Copy link
Collaborator

DanAlbert commented Jul 8, 2021 •
edited
Loading

I can do the work here if that's preferred (and I assume it is), but I wanted to make sure that the patch would be welcome first, since it'd take a sizable time commitment to create.

We (DCS Liberation) hit a not small number of bugs where we have type mismatches between our code and pydcs. mypy does its best for libraries without complete type info, but it's pretty common for anything imported that isn't sufficiently typed to end up degrading to Any, at which point mypy does nothing.

What I'd like to do is send a (probably quite large, but also should be quite straightforward) PR that adds any missing type annotations and enforces disallow_untyped_defs = True in mypy.ini, and make sure that runs as part of the github actions.

Any objections? Type annotations can take a bit of work to maintain, but IME it's quite small, especially when compared to the number of bugs they catch.

Todo items:

  • Fix existing mypy issues.
  • Run mypy in the github action.
  • Fix terrain exporter to include necessary annotations, re-export all terrains (terrain files were disabled as part of step 1)
  • Increase strictness in mypy.ini
  • Add a py.typed file to the package to expose typing information to dependents
@rp-
Copy link
Collaborator

rp- commented Jul 8, 2021

yes please, go ahead!

DanAlbert added a commit to DanAlbert/dcs that referenced this issue Jul 9, 2021
@DanAlbert
Fix existing mypy issues.
0ec449f 
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).
DanAlbert added a commit to DanAlbert/dcs that referenced this issue Jul 9, 2021
@DanAlbert
Run mypy as part of the github check.
42ab6d6 
Part of pydcs#160.
@DanAlbert DanAlbert mentioned this issue Jul 9, 2021
Merged
@DanAlbert
Copy link
Collaborator Author

#161 addresses the first two todo items.

DanAlbert added a commit to DanAlbert/dcs that referenced this issue Jul 9, 2021
@DanAlbert
Fix existing mypy issues.
f65f6d6 
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).
DanAlbert added a commit to DanAlbert/dcs that referenced this issue Jul 9, 2021
@DanAlbert
Run mypy as part of the github check.
fad763e 
Part of pydcs#160.
DanAlbert added a commit to DanAlbert/dcs that referenced this issue Jul 9, 2021
@DanAlbert
Run mypy as part of the github check.
dda3256 
Part of pydcs#160.
DanAlbert added a commit to DanAlbert/dcs that referenced this issue Jul 9, 2021
@DanAlbert
Fix existing mypy issues.
24a4cf5 
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).
DanAlbert added a commit to DanAlbert/dcs that referenced this issue Jul 9, 2021
@DanAlbert
Run mypy as part of the github check.
f24a442 
Part of pydcs#160.
DanAlbert added a commit to DanAlbert/dcs that referenced this issue Jul 9, 2021
@DanAlbert
Fix existing mypy issues.
5815dbf 
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).
DanAlbert added a commit to DanAlbert/dcs that referenced this issue Jul 9, 2021
@DanAlbert
Run mypy as part of the github check.
24335d5 
Part of pydcs#160.
rp- pushed a commit that referenced this issue Jul 9, 2021
@DanAlbert @rp-
Fix existing mypy issues.
69377a0 
Part of #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- pushed a commit that referenced this issue Jul 9, 2021
@DanAlbert @rp-
Run mypy as part of the github check.
2b4eb8b 
Part of #160.
DanAlbert added a commit to DanAlbert/dcs that referenced this issue Jul 9, 2021
@DanAlbert
Add py.typed to expose types to dependents.
ee2775e 
Part of pydcs#160
@DanAlbert DanAlbert mentioned this issue Jul 9, 2021
Merged
rp- pushed a commit that referenced this issue Jul 11, 2021
@DanAlbert @rp-
Add py.typed to expose types to dependents.
1fa1538 
Part of #160
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@DanAlbert @rp-