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

DM-24780: add initial mypy support. #276

Merged
merged 6 commits into from May 8, 2020
Merged

DM-24780: add initial mypy support. #276

merged 6 commits into from May 8, 2020

Conversation

TallJimbo
Copy link
Member

Copying from #273, where this work started:

The setup here is probably a little unusual, because it turned out to be quite tricky to get mypy to determine that the root of our package tree is lsst (which has no __init__.py), not python (which also has no __init__.py) or daf (the first directory that does have such a file). The only solution I could find was to run mypy from the python directory, with a config that enables namespace package support.

This was difficult to diagnose because if you run mypy on files or directories, it mostly doesn't care what the package root is, and seems to be working - but it will not obey configuration that's targeted at a particular directory, and AFAICT the config option that's supposed to warn about that (warn_unused_configs = True) didn't actually warn (maybe it doesn't work on globs?). Anyhow, if you run it with -p lsst.daf.butler instead of on files/directories, it will complain if it doesn't recognize the package, so that seems much safer.

Anyhow, the config disables checking for all of lsst.*, then re-enables it for just lsst.daf.butler.registry.interfaces.*; we can enable other sub-packages as we clean them up.

Copy link
Member

@rra rra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. For the record, here's the flags that I usually work towards in mypy (which does not imply that you should use them all from the start):

disallow_untyped_defs = True
disallow_incomplete_defs = True
strict_equality = True
warn_redundant_casts = True
warn_unreachable = True
warn_unused_ignores = True

Comment on lines +140 to +141
return specs._make(self.addTable(name, spec) # type: ignore
for name, spec in zip(specs._fields, specs)) # type: ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like there's probably something better than # type: ignore, but collections.namedtuple is very weird in the type system, and I couldn't come up with anything better either.

@TallJimbo
Copy link
Member Author

Thanks for the tips. Those stricter flags just required a lot of -> None and **kwargs: Any, so I've turned them on (and I'm glad to know now that those should be habits).

@TallJimbo TallJimbo merged commit 4a27313 into master May 8, 2020
@TallJimbo TallJimbo deleted the tickets/DM-24780 branch May 8, 2020 15:34
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.

None yet

2 participants