-
Notifications
You must be signed in to change notification settings - Fork 18
BK: Add tox.ini with typing,lint (codespell); add workflows for codespell; update pyproject.toml #45
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
base: main
Are you sure you want to change the base?
BK: Add tox.ini with typing,lint (codespell); add workflows for codespell; update pyproject.toml #45
Changes from 13 commits
027e188
ff0f93d
ca67a1a
131d0cd
4575281
6cbde9d
537b98b
6853d5f
26b65c6
afe874f
a1d9e60
cd2a860
60bfa81
fb628c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| [codespell] | ||
| skip = .venv,venvs,.git,build,*.egg-info,*.lock,.asv,.mypy_cache,.tox,fixtures,_version.py,*.pem,versioneer.py | ||
| # ignore-words-list = | ||
| # exclude-file = |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| name: Linters | ||
|
|
||
| on: | ||
| - push | ||
| - pull_request | ||
|
|
||
| jobs: | ||
| lint: | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - name: Set up environment | ||
| uses: actions/checkout@v1 | ||
| with: # no need for the history | ||
| fetch-depth: 1 | ||
| - name: Set up Python | ||
| uses: actions/setup-python@v1 | ||
| with: | ||
| python-version: '3.7' | ||
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| python -m pip install --upgrade tox | ||
| - name: Run linters | ||
| run: | | ||
| tox -e lint | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| # This workflow is running types checks using `tox -e typing` you could do locally. | ||
| # As type hinting is not yet fully and consistently done through out the project only | ||
| # some (randomly) selected files (listed in tox.ini) will be checked. Feel welcome | ||
| # to add extra files to the list there. See https://github.com/datalad/datalad/issues/6884 | ||
| # for the overall goal/progress towards type hints coverage in DataLad. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is unclear to me how type-annotation progress in datalad is a concern for extension authors.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jwodder can correct me, but my limited understanding is that without type annotation of datalad interfaces, we simply cannot achieve complete type annotation/checking within the extension which would use those interfaces. |
||
| name: Type-check | ||
|
|
||
| on: | ||
| - push | ||
| - pull_request | ||
|
|
||
| jobs: | ||
| typing: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Check out repository | ||
| uses: actions/checkout@v2 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Set up Python | ||
| uses: actions/setup-python@v2 | ||
| with: | ||
| python-version: '3.7' | ||
yarikoptic marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| python -m pip install --upgrade tox | ||
| - name: Run type checker | ||
| run: tox -e typing | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,3 +10,5 @@ docs/build | |
| docs/source/generated | ||
| build/ | ||
| dist/ | ||
| .idea/ | ||
| venvs/ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| from datalad.tests.utils_pytest import assert_result_count | ||
|
|
||
|
|
||
| def test_register(): | ||
| def test_register() -> None: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know why this was done, but if it turns out to be required now to type-annotate any test function with
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be noted that these two are not equivalent in regards to typing: no annotation means a return type of "Any". Since test_* functions in pytest should always return None this might seem superfluous, but if the test code is to be type-checked then these annotations are necessary. I also do not see how this would be another barrier to writing tests; if someone forgot to add the return type they will still write a test first and be nagged by the typing workflow to add a return type later.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. However, my comment is focused on whether extension developers must write type annotated tests. As you point out, all of them would need this. But which practical problem is solved with this additional investment. I am specifically talking about test functions, not type-annotation in general.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO type annotation in the tests would help with what type annotation helps in general - ensuring that we have expected types/know what types any particular code is working with. To some degree tests type annotation increases the benefit from those tests. So the 'practical problem' being solved is the same as solved by tests themselves -- ensuring that code is working as expected (returns expected types etc).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Somewhat recent versions of pytest emit a warning on return of something other than None from test functions. In the future this will become an error: pytest-dev/pytest#7337. In that regard, type annotations seem a bit redundant, other than making this implicit assumption explicit in the code (which, honestly, is one of the main reasons to type annotate anyway). There are some test functions in the datalad test suite on maint which still yield values, e.g.: https://github.com/datalad/datalad/blob/6c2a573e9cf75f62b4aa8b03fdd346dca8d29820/datalad/tests/test_tests_utils.py#L134. I don't really see a reason not to be consistent and just type annotate everything though.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ha -- cool find. Just FTR: we are ok/safe -- that file is for testing old
how to do that? i.e. that there is some
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If the yielding function would have been annotated with a return type of None, then mypy would emit this error: that is, the annotated return type and the real return type do not match. |
||
| import datalad.api as da | ||
| assert hasattr(da, 'hello_cmd') | ||
| assert_result_count( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,8 @@ | ||
| [build-system] | ||
| # wheel to get more lightweight (not EASY-INSTALL) entry-points | ||
| requires = ["setuptools >= 43.0.0", "wheel"] | ||
|
|
||
| [tool.isort] | ||
| force_grid_wrap = 2 | ||
| include_trailing_comma = true | ||
| multi_line_output = 3 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,8 +24,10 @@ include = datalad_helloworld* | |
| [options.extras_require] | ||
| # this matches the name used by -core and what is expected by some CI setups | ||
| devel = | ||
| pytest | ||
| coverage | ||
| mypy | ||
| pytest | ||
|
|
||
|
|
||
| [options.entry_points] | ||
| # 'datalad.extensions' is THE entrypoint inspected by the datalad API builders | ||
|
|
@@ -51,3 +53,16 @@ show_missing = True | |
| omit = | ||
| # versioneer code | ||
| datalad_helloworld/_version.py | ||
|
|
||
| # We need to ignore all datalad imports since datalad | ||
| # is not yet typed overall. Unfortunately documented in | ||
| # https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports | ||
| # datalad-specific setting didn't work so we set for all imports. | ||
| # TODO: remove whenever datalad is typed. | ||
| # [mypy-datalad.*] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am unfamiliar with this setup, but it looks as if the "need to ignore all datalad imports" might be commented out below? It is not obvious to me how this is done. |
||
| [mypy] | ||
| exclude = _version\.py$ | ||
| ignore_missing_imports = True | ||
|
|
||
| [mypy-datalad_helloworld._version] | ||
| follow_imports = skip | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| # | ||
| # This file defines common developer helper commands, such as | ||
| # - py3: perform unittest-ing using pytest | ||
| # - lint: codespell the code to avoid typos | ||
| # - flake8: conform code to PEP8 and beyond (not enabled by default) | ||
| # - typing: ensure consistency in type annotations | ||
| # | ||
| # Moveover configuration for pytest and flake8 specified in this file | ||
| # as well, so would be in effect even if used not through tox. | ||
| # | ||
| [tox] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect that this file would need substantial tailoring when adopted for a specific extension. I would appreciate some comments on what is relevant and what not. Right now this communicates to extension developers that they will have to learn about
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added some in now a4d26c4 . Check out if that clarifies it a bit. |
||
| envlist = py3,lint,typing | ||
| #,flake8 | ||
|
|
||
| [testenv:py3] | ||
| changedir = __testhome__ | ||
| commands = pytest -c ../tox.ini -v {posargs} --pyargs datalad_helloworld | ||
| extras = devel | ||
| passenv=HOME | ||
| setenv= | ||
| DATALAD_LOG_LEVEL=DEBUG | ||
|
|
||
| [testenv:lint] | ||
| skip_install = true | ||
| deps = | ||
| codespell~=2.0 | ||
| commands = | ||
| codespell | ||
|
|
||
| [testenv:flake8] | ||
| commands = flake8 datalad_helloworld | ||
|
|
||
| [testenv:typing] | ||
| extras = devel | ||
| commands = | ||
| mypy datalad_helloworld | ||
|
|
||
| [testenv:venv] | ||
| commands = {posargs} | ||
|
|
||
| [pytest] | ||
| filterwarnings = | ||
| markers = | ||
| fail_slow | ||
| githubci_osx | ||
| githubci_win | ||
| integration | ||
| known_failure | ||
| known_failure_githubci_osx | ||
| known_failure_githubci_win | ||
| known_failure_osx | ||
| known_failure_windows | ||
| network | ||
| osx | ||
| probe_known_failure | ||
| serve_path_via_http | ||
| skip_if_adjusted_branch | ||
| skip_if_no_network | ||
| skip_if_on_windows | ||
| skip_if_root | ||
| skip_known_failure | ||
| skip_nomultiplex_ssh | ||
| skip_ssh | ||
| skip_wo_symlink_capability | ||
| slow | ||
| turtle | ||
| usecase | ||
| windows | ||
| with_config | ||
| with_fake_cookies_db | ||
| with_memory_keyring | ||
| with_sameas_remotes | ||
| with_testrepos | ||
| without_http_proxy | ||
|
|
||
| [flake8] | ||
| #show-source = True | ||
| # E265 = comment blocks like @{ section, which it can't handle | ||
| # E266 = too many leading '#' for block comment | ||
| # E731 = do not assign a lambda expression, use a def | ||
| # W293 = Blank line contains whitespace | ||
| #ignore = E265,W293,E266,E731 | ||
| max-line-length = 120 | ||
| include = datalad_helloworld | ||
| exclude = .tox,.venv,venv-debug,build,dist,doc,git/ext/ | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to communicate what this workflow does, and where to look for answers when it fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?