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

[SUPPORT] How to Check Test files by Mypy CI #15

Closed
LovelyBuggies opened this issue May 26, 2020 · 9 comments
Closed

[SUPPORT] How to Check Test files by Mypy CI #15

LovelyBuggies opened this issue May 26, 2020 · 9 comments
Assignees
Labels
question Further information is requested

Comments

@LovelyBuggies
Copy link
Collaborator

Describe your questions

I have used mypy according to https://scikit-hep.org/developer/style#type-checking-new. My pre-commit profig file is like this:

repos:
- repo: https://github.com/psf/black
  rev: 19.10b0
  hooks:
  - id: black
- repo: https://github.com/pre-commit/pre-commit-hooks
  rev: v2.5.0
  hooks:
  - id: check-added-large-files
  - id: mixed-line-ending
  - id: trailing-whitespace
  - id: check-merge-conflict
  - id: check-case-conflict
  - id: check-symlinks
  - id: check-yaml
- repo: https://github.com/pre-commit/mirrors-mypy
  rev: v0.770
  hooks:
  - id: mypy
    files: all  # I tried tests and src, but none worked

I deliberately put a type error in a test file, but pre-commit did not find it. How to make mypy CI work for unit tests?

@LovelyBuggies LovelyBuggies added the question Further information is requested label May 26, 2020
@LovelyBuggies LovelyBuggies changed the title [SUPPORT]: How to Check Test files by Mypy CI [SUPPORT] How to Check Test files by Mypy CI May 26, 2020
@henryiii
Copy link
Member

henryiii commented May 26, 2020

Try adding this to your setup.cfg:

[mypy]
warn_unused_configs = True
pretty = True
files = src
check_untyped_defs = True

I don't think you need the files selection in the MyPy hook above. The important line is the last one, I bet that will start showing a few errors. Normally MyPy only looks inside functions with at least one annotation.

@henryiii
Copy link
Member

Also note that when MyPy runs, it will be in a custom environment, so if you want any things added to the environment, you will have to list them. Probably won't need anything. special, though, at least until boost-histogram provides external typing.

@henryiii
Copy link
Member

You will need to ignore missing imports for now, such as:

[mypy-boost_histogram]
ignore_missing_imports = True

@LovelyBuggies
Copy link
Collaborator Author

LovelyBuggies commented May 27, 2020

@henryiii Thx! After I modify the setup.cfg, it reminds me of places where I should add annotations (mostly the variables I used for multiple times) and some internal type conversions (I have to admit that this is so helpful). But I doubt whether this really works.

I conducted an experiment to testify its functionality. I used the wrong type in test_name.py like

h = NamedHist(axis.Regular("10", 0, 1, name="x")).fill(x=[0.35, 0.35, 0.45]) # bin should be int

and added function annotations for Regular axis __init__ method like

    def __init__(
        self,
        bins: int,
        start: int,
        stop: int,
        ....
    ):

But this error escapes from the check of mypy pre-commit check. Do U have any idea why it could?

@LovelyBuggies
Copy link
Collaborator Author

You will need to ignore missing imports for now, such as:

[mypy-boost_histogram]
ignore_missing_imports = True

We could pass the pre-commit, if it doesn't contain other errors, without missing imports currently (BH's type hints seem not to be considered by mypy right now). So I am not sure about the usage of this line.

Nevertheless, it doesn't hurt and I have added it :)

@henryiii
Copy link
Member

We don't run MyPy on the tests. Generally, units tests may test things that are not allowed by a type checker. For example, you may test passing a float or a string to bins to see what error is thrown (and an exact float might be supported instead of an int), but the type checker will request a user put an int here.

We probably could run the type checker on examples, but I think we have to add "py.typed" files in all directories in src/hist to get that to work (that tells python that this package supports type checking by things outside this package).

@henryiii
Copy link
Member

Break your assumption of bins: int, etc from inside the __init__ function, and MyPy should catch it. bins + "hi" or something like that. Or if you break it from somewhere else in src/hist (assuming you either have check_untyped_defs on or you also have at least one annotation on the function you break it from).

@LovelyBuggies
Copy link
Collaborator Author

We don't run MyPy on the tests.

Maybe I'm doing the right thing, but have no idea how to make some counterexamples to test the functionality of mypy CI.

LovelyBuggies added a commit that referenced this issue Jun 1, 2020
* NamedHist init

* black reformat

* allow Hist axes not to have names

* ToDo: forbid non-named NamedHist

* Update named.py

* better-styled derive

* forbid no-named/duplic axes for NamedHist

* force names everywhere

* add mypy to pre-commit

* Update NamedHist.ipynb

* Update dev-environment.yml

* add tests of NamedHist

* Update setup.cfg

* Update dev-environment.yml

* Update mypy CI check in #15

* rm py3.6 for annotations

* solved #13 (comment)

* typing hints added

* add basic_tests for StrCategory

* add py3.6

* numpy to core.py

* rm __future__.annotations

* correct regular expression of valid Py identifier

* add tests for exceptions
@LovelyBuggies
Copy link
Collaborator Author

Looks good, currently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants