add mypy support for linters/testing

[Arnatious]

Add support for mypy based static type checking, in the vein of ament_flake8 and ament_pyflakes.

Signed-off-by: Ted Kern ted.kern@canonical.com

[kyrofa]

This looks great @Arnatious, thank you! A couple of small changes suggested, but overall this seems to work well at least for the python projects on which I've been testing it (I haven't tested the cmake bits). It's also missing tests.

[dirk-thomas]

Imo this shouldn't be merged with the pending "admittedly-not-ideal-but-hopefully-works-in-most-common-cases" approach.

[kyrofa]

Imo this shouldn't be merged with the pending "admittedly-not-ideal-but-hopefully-works-in-most-common-cases" approach.

Wait, didn't we agree that there was no better way to do that? What exactly would you like to see?

[dirk-thomas]

didn't we agree that there was no better way to do that? What exactly would you like to see?

Yes, we don't see a better way to do that atm. Imo that doesn't mean that the approach is good enough to be merged into master which will become the next stable release. I think it doesn't match the quality demand we have for ROS if the fragile heuristic breaks in certain use cases.

If other reviewers have a different opinion and think this should be merged anyway please feel free to comment here.

[kyrofa]

I think it doesn't match the quality demand we have for ROS if the fragile heuristic breaks in certain use cases.

To be clear, if someone puts the build and install spaces somewhere other than the default, the side effect is that mypy won't find all ros2 imports, which can be ignored by the project. It doesn't break anything, just makes mypy not as useful. The only alternatives (such as not supporting --symlink-install by only using COLCON_PREFIX_PATH directly instead of its parent directory, or not following imports at all) can stop making this assumption, at the cost of mypy not finding all ros2 imports in MORE cases. Would you prefer that? We're fine with that, it just makes it less useful. Let us know which option you prefer.

There are multiple ROS packages today using the static typing, but without the ability to check them in CI they get out of date to the point of being harmful (for example, rosidl_python will fail mypy checks). Getting this into master will increase quality of the overall ecosystem, not decrease it.

[dirk-thomas]

if someone puts the build and install spaces somewhere other than the default, the side effect is that mypy won't find all ros2 imports, which can be ignored by the project. It doesn't break anything, just makes mypy not as useful.

The developer expects that the linting is happening and for whatever reason it doesn't perform the checks as intended. So it does break user expectation.

If the linter doesn't find the right paths it might not generate a warning where one would be expected. I don't think that case should be silently ignored. At the very least the linter should respond with a skipped check (probably in addition to the ones it was able to perform) in the generated report. Otherwise the user has no indication whatsoever that the linting was not completed as intended.

[kyrofa]

If the linter doesn't find the right paths it might not generate a warning where one would be expected.

mypy will raise an error indeed, and the project has the option to ignore it in a mypy.ini file. This is how mypy works, @dirk-thomas. Would you prefer that this linter only finds ros2 imports that are contained within COLCON_PREFIX_PATH instead, then? Or would you prefer that this linter not follow imports at all and only checks the files of the project being linted, and not how it uses its dependencies (that gets rid of the whole MYPYPATH thing)? Both are valid options that gets rid of the assumption.

[dirk-thomas]

Please make sure to run the tests locally and address the current linter failures.

This comment still applies.

[dirk-thomas]

Linux CI only testing ament_mypy:

[Arnatious]

Linux CI only testing ament_mypy:

@dirk-thomas I'm getting different results? I'm running a source checkout of dashing atm

ubuntu@ros2-dev  ~/ros2  colcon test --packages-select ament_mypy --event-handlers console_direct+ --pytest-args --cov=ament_mypy --cov-report term-missing
Starting >>> ament_mypy
running pytest                           
running egg_info
writing /home/ubuntu/ros2/build/ament_mypy/ament_mypy.egg-info/PKG-INFO
writing dependency_links to /home/ubuntu/ros2/build/ament_mypy/ament_mypy.egg-info/dependency_links.txt
writing entry points to /home/ubuntu/ros2/build/ament_mypy/ament_mypy.egg-info/entry_points.txt
writing requirements to /home/ubuntu/ros2/build/ament_mypy/ament_mypy.egg-info/requires.txt
writing top-level names to /home/ubuntu/ros2/build/ament_mypy/ament_mypy.egg-info/top_level.txt
reading manifest file '/home/ubuntu/ros2/build/ament_mypy/ament_mypy.egg-info/SOURCES.txt'
writing manifest file '/home/ubuntu/ros2/build/ament_mypy/ament_mypy.egg-info/SOURCES.txt'
running build_ext                        
============================= test session starts ==============================
platform linux -- Python 3.6.8, pytest-5.0.1, py-1.8.0, pluggy-0.12.0
cachedir: /home/ubuntu/ros2/build/ament_mypy/.pytest_cache
rootdir: /home/ubuntu/ros2/src/ament/ament_lint/ament_mypy
plugins: ament-flake8-0.7.4, ament-lint-0.7.4, ament-xmllint-0.7.4, ament-pep257-0.7.4, ament-copyright-0.7.4, rerunfailures-7.0, repeat-0.8.0, cov-2.7.1, mock-1.7.1
collecting ...                           
collected 11 items                                                             

test/test_ament_mypy.py .........                                        [ 81%]
test/test_flake8.py .                                                    [ 90%]
test/test_mypy.py .                                                      [100%]

------ generated xml file: /home/ubuntu/ros2/build/ament_mypy/pytest.xml -------

----------- coverage: platform linux, python 3.6.8-final-0 -----------
Name                     Stmts   Miss  Cover   Missing
------------------------------------------------------
ament_mypy/__init__.py       0      0   100%
ament_mypy/main.py         109      0   100%
------------------------------------------------------
TOTAL                      109      0   100%

========================== 11 passed in 4.16 seconds ===========================
Finished <<< ament_mypy [7.58s]          

Summary: 1 package finished [9.61s]
 ubuntu@ros2-dev  ~/ros2  ```

[Arnatious]

@dirk-thomas Those jenkins failures are all related to pytest-mock

[dirk-thomas]

Those jenkins failures are all related to pytest-mock

I added the pytest-mock dependency to the existing CI PR: ros2/ci@173e55e

With that the CI status on Linux looks good:

Is the plan to get this merged as is and iterate on enhancements like include Python modules outside of the tested package in a separate PR?

[Arnatious]

@dirk-thomas that's the plan, a longer discussion will need to be had about how to traverse the imports and how that fits with mypy's expectation of "all local code and imported packages should pass or else you'll need to micromanage."

[dirk-thomas]

• Linux-aarch64
• macOS
• Windows

[dirk-thomas]

• Windows

[Arnatious]

@dirk-thomas should I squash these commits prior to any merge?

[dirk-thomas]

should I squash these commits prior to any merge?

No need for that - I will use the "Squash and merge" option of the GitHub UI.

Thank you for contributing mypy support!

[wjwwood]

Is there a recommended way to add this to a pure Python package (like launch)? It might be a good thing to have a snippet for in this file: https://github.com/ament/ament_lint/blob/master/ament_mypy/doc/index.rst

[Arnatious]

@wjwwood aye, that's a good idea
For now, a good reference is looking at how it tests itself + how it uses flake8, I'll prep up a snippet about this ASAP.

[kyrofa]

@wjwwood while waiting for better docs you can also refer to this SROS 2 PR. It's the simplest use-case, no args, config file or anything, but docs will outline how to do that better.

[jacobperron]

I think there's missing install instructions for the default configuration file, ament_mypy.ini, which is causing test failures in ros2/sros2#154.

[jacobperron]

Windows debug builds are failing with a mypy-related error:
Looks like we might need to build a debug wheel for mypy.

[jacobperron]

Looks like we might need to build a debug wheel for mypy.

See ros2/ci#333 for a follow-up.