Improve type annotation of codebase, add CI for mypy, fix a string format in test #191

a-detiste · 2023-11-12T19:15:48Z

This is just an example on how/why add annotations, they enable a new kind of linting with mypy; they also make it easier for newcomers to contribute to bigger projects;

To do it more logicaly, I would first need to spend more time on citeproc-py, but I see there's globaly much more to do on this other project. (but I cheated with sudo touch /usr/lib/python3/dist-packages/citeproc/py.typed)

this includes a workaround to a mypy bug: python/mypy#1465

+ workaround a mypy bug: python/mypy#1465

codecov-commenter · 2023-11-12T19:16:25Z

Codecov Report

Attention: 83 lines in your changes are missing coverage. Please review.

Comparison is base (581bce8) 82.35% compared to head (f3dc4f5) 81.00%.
Report is 6 commits behind head on master.

Files	Patch %	Lines
duecredit/collector.py	75.00%	11 Missing ⚠️
duecredit/io.py	78.84%	10 Missing and 1 partial ⚠️
duecredit/utils.py	61.53%	5 Missing ⚠️
duecredit/log.py	87.09%	2 Missing and 2 partials ⚠️
duecredit/dueswitch.py	75.00%	2 Missing and 1 partial ⚠️
duecredit/versions.py	76.92%	3 Missing ⚠️
duecredit/entries.py	88.88%	1 Missing and 1 partial ⚠️
duecredit/injections/injector.py	91.66%	1 Missing and 1 partial ⚠️
duecredit/injections/mod_biosig.py	50.00%	1 Missing and 1 partial ⚠️
duecredit/injections/mod_dipy.py	50.00%	1 Missing and 1 partial ⚠️
... and 19 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
- Coverage   82.35%   81.00%   -1.35%     
==========================================
  Files          47       47              
  Lines        2420     2517      +97     
  Branches      339      365      +26     
==========================================
+ Hits         1993     2039      +46     
- Misses        356      383      +27     
- Partials       71       95      +24

Flag	Coverage Δ
unittests	`80.85% <77.83%> (-1.39%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yarikoptic · 2023-11-12T19:36:29Z

but I cheated with sudo touch /usr/lib/python3/dist-packages/citeproc/py.typed

yikes. didn't look in detail but may be you just needed a config like https://mypy.readthedocs.io/en/stable/config_file.html#confval-ignore_missing_imports ?

yarikoptic · 2023-11-12T19:38:47Z

BTW, checkout references to automated tools in datalad/datalad#6884 (comment) - might come handy

a-detiste · 2023-11-12T19:42:54Z

sudo touch py.typed: I do it for two reasons:

let mypy have a peek inside the libs to see what happens
track what' I'm busy with this way:

$ sudo cruft | grep typed
        /usr/lib/python3/dist-packages/citeproc/py.typed
        /usr/lib/python3/dist-packages/debconf/py.typed
        /usr/lib/python3/dist-packages/tap/py.typed

Then when someday the packages start providing a real py.typed, the corresponding entry will fall out of the list
(disclaimer: I'm maintainer for cruft)

yarikoptic · 2023-11-12T20:04:59Z

FWIW, an advice from a Debian developer -- use venv environments while developing and don't "abuse" sudo and do not modify system-wide, installed via package manager components ;)

a-detiste · 2023-11-12T20:11:04Z

cruft purpose is to recover from these kinds of mishaps... I'm still doing the autopsy of a Wheezy filesystem upgraded to Buster and shiped as-is by our supplier who edited random files by hand in /etc and elsewhere ... but yes people should use venv
(I still do need a somewhat dirty system to feed the cleaning monitoring cron jobs...)

duecredit/entries.py

…tated files

yarikoptic · 2023-11-16T19:36:41Z

ok, thank you for the effort!

to add typing we should test typing ;) I am ok to limit for now to just this selected set of files, but if you would like challenge -- try to annotate more (then adjust tox.ini to remove skips and subselection of modules)

$ mypy --ignore-missing-imports . Success: no issues found in 52 source files

this useless parameter confuse me

a-detiste · 2023-11-17T02:02:20Z

all files can be included in the test already (there was just a 1 line glitch in setup.py)

--strict mode can be enabled later

a-detiste · 2023-11-17T12:01:10Z

I'm packing for an Holliday & will be offline ... second leg of the trip is DebCamp in Cambridge on Thursday...
So you might merge this as-is as & I'll resume later on.

yarikoptic · 2023-11-17T16:45:17Z

.github/workflows/typing.yml

+          python -m pip install --upgrade tox
+
+      - name: Run type checker
+        run: tox -e typing


FWIW -- we better adopt workflow like https://github.com/dandi/zarr_checksum/blob/2a30d1b482e96aa475f0a480f813be6ab8f9a1c9/.github/workflows/test.yml -- I like how it centralizes other testing as well to that single workflow + tox. WDYT?

yarikoptic · 2023-11-17T16:48:39Z

@jwodder -- would you be so kind to review this PR and provide your recommendations for further typing annotation in this project?

yarikoptic · 2023-11-17T16:49:09Z

meanwhile, as green and per above comment -- let's merge. Feedback will be considered for future work.

a-detiste · 2023-11-17T16:57:38Z

pyupgrade can automaticaly replace the Optional and Union later when baseline is bumped.

a-detiste · 2023-11-17T17:11:42Z

the py.typed flag can now be added to this project (and included in release artefacts)

jwodder · 2023-11-17T16:53:28Z

duecredit/collector.py

@@ -11,6 +11,7 @@
 import os
 import sys
 from functools import wraps
+from typing import Any, Dict, List, Optional, Tuple, Union


I would strongly recommend dropping support for Python 3.6 (It's been EOL for almost two years), after which you can add from __future__ import annotations to all the source files and then change annotations like Union[None, str, Tuple[str, str]] to None | str | tuple[str, str], get rid of the quotes around some other annotations, and get rid of most typing imports.

I guess we could make use of https://github.com/asottile/pyupgrade here indeed. Submitted #194

Thank you very much for this review. I greatly enjoy learning new tricks.

I found this great reference that match what you said about from __future ... that I did t full grasped at first time:

https://adamj.eu/tech/2021/05/21/python-type-hints-how-to-upgrade-syntax-with-pyupgrade/

jwodder · 2023-11-17T16:56:46Z

duecredit/collector.py

+        path: Optional[str] = None,
+        version: Union[None, str, Tuple[str, str]] = None,
+        cite_module: bool = False,
+        tags: List[str] = ['implementation']


Using a list — a mutable structure — as a default value to a Python function is a well-known footgun. Do one of the following instead:

Change tags to tags: Optional[List[str]] = None and, inside the function, set tags to ["implementation"] if it's None.

Change tags to tags: Sequence[str] = ("implementation",) (Note the trailing comma to create a one-element tuple.) If this breaks any of your code, then using a list here was a bug to begin with.

jwodder · 2023-11-17T17:03:23Z

duecredit/collector.py

@@ -141,7 +149,7 @@ def objname(self):
        else:
            return None

-    def __contains__(self, entry):
+    def __contains__(self, entry: 'Citation') -> bool:  # Self PEP673


If, as the comment suggests, entry is only supposed to be of the exact same type as self (not even allowing for subclassing), then it should be annotated with Self instead of "Citation". As Self is only used as an annotation, and if you take my advice above about from __future__ import annotations, you can import Self by simply adding this to the top of the file:

from typing import TYPE_CHECKING if TYPE_CHECKING: from typing_extensions import Self

typing_extensions is a dependency of mypy, so you won't even have to adjust the project's dependencies!

jwodder · 2023-11-17T17:04:08Z

duecredit/collector.py

@@ -253,7 +268,8 @@ def cite(self, entry, **kwargs):
            citation = self.citations[citation_key]
        except KeyError:
            self.citations[citation_key] = citation = Citation(entry_, **kwargs)
-        assert(citation.key == citation_key)
+        assert type(citation) is Citation


Unless you want to prohibit subclassing, this would be better written as assert isinstance(citation, Citation).

jwodder · 2023-11-17T17:05:51Z

duecredit/collector.py

@@ -454,7 +479,11 @@ def __init__(self, collector, outputs="stdout,pickle", fn=DUECREDIT_FILE):
        ]

    @staticmethod
-    def _get_output_handler(type_, collector, fn=None):
+    def _get_output_handler(


I suggest annotating type_ as Literal["stdout", "stderr", "pickle"] and using @typing.overload to select the correct return type.

Or an Enum, I don't know if this is a public API

jwodder · 2023-11-17T17:13:35Z

duecredit/io.py

-                any(filter(lambda x: x.cite_module, package_citations)) or \
-                any(filter(lambda x: _is_contained(package, x), cited_modobj)):
+                any(filter(lambda x: x.cite_module, package_citations)) or any(filter(lambda x: _is_contained(package, x), cited_modobj)):  # type: ignore
+                # WIP, mypy doesn(t understand the 'ignore' on a continuated line


"Doesn't" is spelled with an apostrophe, not a parenthesis.

jwodder · 2023-11-17T17:16:29Z

duecredit/io.py

+# https://github.com/python/mypy/issues/9242#issuecomment-667586397
+# https://github.com/spack/spack/pull/35640
+if sys.platform == "win32":
+    OSExceptions = (OSError, WindowsError)


WindowsError has been a subclass of OSError since Python 3.3. Just use OSError by itself.

jwodder · 2023-11-17T17:18:28Z

duecredit/tests/test__main__.py


 from .. import __main__, __version__
 from .. import due

+if TYPE_CHECKING:


Why guard this behind if TYPE_CHECKING? Unless you're supporting pre-7.0 pytest (Why would you?), you can just import these types unconditionally.

jwodder · 2023-11-17T17:20:39Z

tox.ini

+    mypy
+    types-requests
+commands =
+    mypy --ignore-missing-imports {posargs} duecredit/


I advise against using --ignore-missing-imports, as will also swallow errors about importing from a module that doesn't exist. If the goal is to deal with dependencies that lack typing information, the better way is to add an override to the mypy config for each such dependency like so:

[mypy-packagename.*] ignore_missing_imports = True

Would it make sense to ship mininimalistic .pyi type hints for cité citeproc and others ?
I mean as an interim solution ?

I don't think so.

jwodder · 2023-11-17T17:22:10Z

duecredit/tests/test_api.py

-def test_noincorrect_import_if_no_lxml_numpy(monkeypatch, kwargs, env, stubbed_env):
+def test_noincorrect_import_if_no_lxml_numpy(
+    monkeypatch: 'MonkeyPatch',
+    kwargs,


These arguments aren't annotated. I suggest adding allow_incomplete_defs = False to the project's mypy configuration so that such problems are caught.

I m working with mypy --strict that list 220-something todo ... these items are not forgtoten

RF: annotate entries.py, fix a string format in test

717bf22

+ workaround a mypy bug: python/mypy#1465

yarikoptic reviewed Nov 16, 2023

View reviewed changes

duecredit/entries.py Show resolved Hide resolved

yarikoptic added 2 commits November 16, 2023 14:34

Remove outdated pythons from tox.ini. add typing testenv to test anno…

874fb2d

…tated files

Add github workflow to trigger tox -e typing

2afd1cd

a-detiste added 9 commits November 16, 2023 23:31

add minimal typing information to setup.py for first iteration

376d9c1

$ mypy --ignore-missing-imports . Success: no issues found in 52 source files

typing: annotate utils.py 1/2

5663ed5

typing: annotate versions.py 1/2, __cmp__ is gone in Py3

5476b39

typing: annotate log.py 100%

4376769

typing: annotate duecredit/io.py 1/2

e596d00

typing: revert glitch introduced in previous commit

ed7aef7

this useless parameter confuse me

typing: parsers.py 100%

bb87e2a

typing: collector.py 1/2

3fb2db9

typing: test_io.py 100%

a179e03

a-detiste added 7 commits November 17, 2023 03:18

tox: run mypy on everything

2d89a91

typing: attemp to fix CI run on Github

add1186

typing: injections

0166595

typing: glitch syntax Union[]

72d41a7

typing: attempt to fix build

e53e355

typing: annotate dueswitch.py

8507621

typing: fix typing of MonkeyPatch

6da688a

typing: io.py: annotate _get_collated_citations WIP

36addc6

a-detiste added 2 commits November 17, 2023 13:44

typing: use TYPE_CHECKING to avoid polluting runtime environment

9b5e010

typing: fix regression in io.py

f3dc4f5

yarikoptic reviewed Nov 17, 2023

View reviewed changes

yarikoptic added the patch Increment the patch version when merged label Nov 17, 2023

yarikoptic changed the title ~~RF: annotate entries.py, fix a string format in test~~ Improve type annotation of codebase, add CI for mypy, fix a string format in test Nov 17, 2023

yarikoptic merged commit 18dca6a into duecredit:master Nov 17, 2023
14 checks passed

jwodder reviewed Nov 17, 2023

View reviewed changes

a-detiste deleted the annotate branch November 23, 2023 11:10

a-detiste mentioned this pull request Nov 23, 2023

misc. move typing improvements, WIP #195

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve type annotation of codebase, add CI for mypy, fix a string format in test #191

Improve type annotation of codebase, add CI for mypy, fix a string format in test #191

a-detiste commented Nov 12, 2023

codecov-commenter commented Nov 12, 2023 •

edited

yarikoptic commented Nov 12, 2023

yarikoptic commented Nov 12, 2023

a-detiste commented Nov 12, 2023

yarikoptic commented Nov 12, 2023 •

edited

a-detiste commented Nov 12, 2023 •

edited

yarikoptic commented Nov 16, 2023

a-detiste commented Nov 17, 2023

a-detiste commented Nov 17, 2023

yarikoptic Nov 17, 2023

yarikoptic commented Nov 17, 2023

yarikoptic commented Nov 17, 2023

a-detiste commented Nov 17, 2023

a-detiste commented Nov 17, 2023

jwodder Nov 17, 2023

yarikoptic Nov 18, 2023

a-detiste Nov 18, 2023

jwodder Nov 17, 2023 •

edited

jwodder Nov 17, 2023

jwodder Nov 17, 2023

jwodder Nov 17, 2023

a-detiste Nov 17, 2023

jwodder Nov 17, 2023

jwodder Nov 17, 2023

jwodder Nov 17, 2023

jwodder Nov 17, 2023 •

edited

a-detiste Nov 17, 2023

jwodder Nov 17, 2023

jwodder Nov 17, 2023

a-detiste Nov 17, 2023

Improve type annotation of codebase, add CI for mypy, fix a string format in test #191

Improve type annotation of codebase, add CI for mypy, fix a string format in test #191

Conversation

a-detiste commented Nov 12, 2023

codecov-commenter commented Nov 12, 2023 • edited

Codecov Report

yarikoptic commented Nov 12, 2023

yarikoptic commented Nov 12, 2023

a-detiste commented Nov 12, 2023

yarikoptic commented Nov 12, 2023 • edited

a-detiste commented Nov 12, 2023 • edited

yarikoptic commented Nov 16, 2023

a-detiste commented Nov 17, 2023

a-detiste commented Nov 17, 2023

Choose a reason for hiding this comment

yarikoptic commented Nov 17, 2023

yarikoptic commented Nov 17, 2023

a-detiste commented Nov 17, 2023

a-detiste commented Nov 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwodder Nov 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwodder Nov 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 12, 2023 •

edited

yarikoptic commented Nov 12, 2023 •

edited

a-detiste commented Nov 12, 2023 •

edited

jwodder Nov 17, 2023 •

edited

jwodder Nov 17, 2023 •

edited