search

douglasrizzo / catsim

Computerized Adaptive Testing Simulator
GNU Lesser General Public License v3.0
124 stars 35 forks source link

Type checking #24

Closed frankier closed 3 years ago

frankier commented 3 years ago

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

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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!