#27 - mypy fixup, adds to #24 - unblocks #31 #32

achimgaedke · 2022-11-19T19:11:04Z

This PR proposes fix the fix_mypy_strict branch:

The 3 commits contain:

Change of the github action to use a "matrix build", simply taken from the examples and adapted to the project
Complete the type annotations in the source code (fixes Re-implement mypy strict checking #27)
Add __all__ to the __init__.py to define the public interface of this package.

The next steps would be to merge the result into master and release it (#31).

achimgaedke · 2022-11-19T20:30:28Z

src/pyprojroot/criterion.py

            if isinstance(c, _PathLike):
                if (path / c).exists():
                    return True
-            else:
+            if callable(c):


I should revert this to else again. Tried to get the type inference of mypy onto the right track.

I ended up giving up here - see the ignore instruction.

I think, ideally, this logic would all be encapsulated with classes. But I think this was just a simple version that works. But making something that's not confusing for mypy shouldn't be the first objective.

Can we leave the else, please? It should throw an error (ideally TypeError but that's not super important) if c isn't path-like or callable, rather than silently ignoring it.

Perhaps it needs a type hint for the criterion_collection variable. Something like List[Union[PathType, Callable[[PathType], bool]].

jamesmyatt · 2022-11-20T10:09:51Z

src/pyprojroot/__init__.py

 from .root import find_root, find_root_with_reason
 from .here import here
+
+__all__ = [


I don't think you need this since everything you want to export is explicitly imported.

jamesmyatt · 2022-11-20T10:10:31Z

src/pyprojroot/criterion.py

@@ -9,35 +9,50 @@
 import typing
 from os import PathLike as _PathLike

+
+_PathType = typing.Union[_PathLike, str]


I think (but I'm not sure) that "PathLike" includes "str". Can you check this?

jamesmyatt · 2022-11-20T10:15:02Z

src/pyprojroot/criterion.py

 # TODO: It would be nice to have a class that encapsulates these checks,
 # so that we can implement methods like |, !, &, ^ operators

 # TODO: Refactor in a way that allows creation of reasons


-def as_root_criterion(criterion) -> typing.Callable:
+def as_root_criterion(
+    criterion: _CriterionType,


This type hint is quite complicated. Maybe it would be better replaced with something using functools.singledispatch: https://docs.python.org/3/library/functools.html#functools.singledispatch. That's probably a lot for this PR though.

But I don't know if it's worth making an alias for this, since it's only used once. The purpose of this function is to take various inputs and standardise them as a callable version.

jamesmyatt · 2022-11-20T13:45:23Z

src/pyprojroot/root.py

    """
    Find directory matching root criterion.

    Recursively search parents of start path for directory
    matching root criterion.
    """
    try:
-        root, _ = find_root_with_reason(criterion, start=start, **kwargs)
+        root, _ = find_root_with_reason(criterion, start=start)


I'd prefer it if the extra kwargs we still passed through, since it ensures future compatibility. What was the mypy error?

jamesmyatt · 2022-11-20T13:46:59Z

src/pyprojroot/criterion.py

+_PathType = typing.Union[_PathLike, str]
+_CriterionType = typing.Union[
+    typing.Callable[[_PathType], bool],
+    typing.Callable[[_pathlib.Path], bool],


I don't think you need to duplicate these for _pathlib.Path, since that's also "PathLike"

jamesmyatt · 2022-11-20T13:48:18Z

src/pyprojroot/root.py



-def as_start_path(start: _PathLike) -> _pathlib.Path:
+def as_start_path(start: _typing.Union[None, _PathType]) -> _pathlib.Path:


I think it's better to write _typing.Optional[_PathType] rather than _typing.Union[None, _PathType]. Although in later python versions, _PathType | None would be preferred.

jamesmyatt · 2022-11-20T13:58:01Z

src/pyprojroot/criterion.py

-def as_root_criterion(criterion) -> typing.Callable:
+def as_root_criterion(
+    criterion: _CriterionType,
+) -> typing.Callable[[_pathlib.Path], bool]:


Probably worth making an alias for this output type, since it's reused.

jamesmyatt · 2022-11-20T13:58:22Z

src/pyprojroot/criterion.py

-    criterion = list(criterion)
+        criterion_collection = [criterion]
+    else:
+


I would remove the extra blank line here: it's unnecessary.

jamesmyatt · 2022-11-20T14:17:46Z

.github/workflows/python-build-test.yml

+          black --check --diff src/pyprojroot tests
+      - name: Test with pytest
+        run: |
+          PYTHONPATH=src pytest


The correct thing to do is to install the package as editable:

python -m pip install -e .

jamesmyatt · 2022-11-20T14:18:10Z

.github/workflows/python-build-test.yml

+      - name: Install dependencies
+        run: |
+          python -m pip install --upgrade pip
+          pip install flake8 pytest mypy black


Would be better if this were synchronised with environment-dev.yml

jamesmyatt · 2022-11-20T14:20:46Z

src/pyprojroot/criterion.py

    if callable(criterion):
        return criterion

    # criterion must be a Collection, rather than just Iterable
    if isinstance(criterion, _PathLike):
-        criterion = [criterion]
-    criterion = list(criterion)
+        criterion_collection = [criterion]


Maybe it's better to just return the right function directly here and then handle the iterable input separately.

def f(path: _pathlib.Path) -> bool: return (path / criterion).exists()

jamesmyatt · 2022-11-20T14:21:59Z

src/pyprojroot/criterion.py

+        criterion_collection = [criterion]
+    else:
+
+        criterion_collection = list(criterion)  # type: ignore[arg-type]


For anyone that's interested, this needs to be converted into a list here from a generic iterable since we need to be able to restart the iterator.

jamesmyatt · 2022-11-20T14:24:12Z

Thanks for your work @achimgaedke . I've put quite a few comments, but remember that I'm not a maintainer: @chendaniely is. So my comments are just opinions.

achimgaedke · 2022-11-20T18:57:47Z

Thanks @jamesmyatt for the comments, I will work through them.

I had a long fight with mypy, some comments are going right down to where I lost the battle... "I think this is required, mypy doesn't think it is sufficient".

Will see...

chendaniely · 2023-02-21T06:47:19Z

tyty @achimgaedke for the PR merged!

I posted a quick reponse in #33 to continue the last bit of the conversatio in this PR. 🙇

Achim Gädke added 3 commits November 19, 2022 23:50

use matrix build/test gh action

1a5a355

make flake happy

17df82b

fixed mypy (mostly)

2f88ae8

achimgaedke changed the title ~~Achims edits~~ #27 - mypy fixup, adds to #24 - unblocks #31 Nov 19, 2022

achimgaedke commented Nov 19, 2022

View reviewed changes

jamesmyatt reviewed Nov 20, 2022

View reviewed changes

chendaniely merged commit 4b6ffd1 into chendaniely:fix_mypy_strict Feb 21, 2023

chendaniely mentioned this pull request Feb 21, 2023

Update + status on repository #33

Open

2 tasks

achimgaedke deleted the achims-edits branch February 21, 2023 22:22

achimgaedke mentioned this pull request Feb 21, 2023

consolidate dev #34

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#27 - mypy fixup, adds to #24 - unblocks #31 #32

#27 - mypy fixup, adds to #24 - unblocks #31 #32

achimgaedke commented Nov 19, 2022 •

edited

Loading

achimgaedke Nov 19, 2022

jamesmyatt Nov 20, 2022 •

edited

Loading

jamesmyatt Nov 20, 2022

jamesmyatt Nov 20, 2022

jamesmyatt Nov 20, 2022

jamesmyatt Nov 20, 2022

jamesmyatt Nov 20, 2022

jamesmyatt Nov 20, 2022 •

edited

Loading

jamesmyatt Nov 20, 2022

jamesmyatt Nov 20, 2022

jamesmyatt Nov 20, 2022

jamesmyatt Nov 20, 2022

jamesmyatt Nov 20, 2022 •

edited

Loading

jamesmyatt Nov 20, 2022

jamesmyatt Nov 20, 2022

jamesmyatt Nov 20, 2022

jamesmyatt Nov 20, 2022

jamesmyatt commented Nov 20, 2022

achimgaedke commented Nov 20, 2022

chendaniely commented Feb 21, 2023



		def as_start_path(start: _PathLike) -> _pathlib.Path:
		def as_start_path(start: _typing.Union[None, _PathType]) -> _pathlib.Path:

#27 - mypy fixup, adds to #24 - unblocks #31 #32

#27 - mypy fixup, adds to #24 - unblocks #31 #32

Conversation

achimgaedke commented Nov 19, 2022 • edited Loading

Choose a reason for hiding this comment

jamesmyatt Nov 20, 2022 • edited Loading

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

jamesmyatt Nov 20, 2022 • edited Loading

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

jamesmyatt Nov 20, 2022 • edited Loading

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

jamesmyatt commented Nov 20, 2022

achimgaedke commented Nov 20, 2022

chendaniely commented Feb 21, 2023

achimgaedke commented Nov 19, 2022 •

edited

Loading

jamesmyatt Nov 20, 2022 •

edited

Loading

jamesmyatt Nov 20, 2022 •

edited

Loading

jamesmyatt Nov 20, 2022 •

edited

Loading