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

Type checking #24

Closed
frankier opened this issue Jun 1, 2021 · 3 comments
Closed

Type checking #24

frankier opened this issue Jun 1, 2021 · 3 comments

Comments

@frankier
Copy link
Contributor

frankier commented Jun 1, 2021

Currently there are type hints in the source code, but it looks like no type checking is being performed. Possible set up steps are:

  • Ensure a type checker is installed as a dev requirement
  • Add type checking as a pre-commit hook
  • Add type checking in GitHub CI

Once the package type checks it can be marked as having types so that downstream packages can use it by adding a py.typed marker file: https://www.python.org/dev/peps/pep-0561/#packaging-type-information

@douglasrizzo
Copy link
Owner

The original idea was for the type hints to help when using a code editor, not to make them a hard requirement for users. In any case, I went ahead and fixed numerous mypy errors. The fixes are in a new branch for now.

There are still type errors related to how catsim interacts with imported packages (I tested against data-science-types and numpy-stubs). I don't know if they are worth the trouble fixing.

I also added mypy to an extras_require installation target on setup.py, although I am not sure that is what you meant by:

Ensure a type checker is installed as a dev requirement

Is this enough for catsim to be marked as having type checks? I am not familiar with PEP 561.

@frankier
Copy link
Contributor Author

frankier commented Jun 4, 2021

Okay! I'd heard that people used type hints this way with e.g. PyCharm, but I suppose I associate them quite strongly with mypy. It's of course up to you how you think they should be used.

The type checker as a dev requirement is unrelated, but I've found it convenient to install checkers this way so that developers using a virtualenv get them installed there, so they can be run in a pre-commit hook, and so they can be run in CI.

The link I sent is directly to the subsection explaining how to package type information. To summarise: add a marker file py.typed and adding this marker to your setup.py to ensure it is installed with the package. It looks like you've done this now though.

The branch looks good to me. I am putting together some PRs. I can rebase them after it's merge if need be.

@douglasrizzo
Copy link
Owner

No need to rebase. I have already merged everything. I also made sure tests are passing (had to migrate them from travis.org to GH Actions). Maybe in the near future we can also use GH Actions to type check.

In any case, thanks for all your recent contributions!

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

No branches or pull requests

2 participants