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.

Sign up for GitHub

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

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

Merged
merged 3 commits into from
Feb 21, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1a5a355
use matrix build/test gh action
Nov 19, 2022
17df82b
make flake happy
Nov 19, 2022
2f88ae8
fixed mypy (mostly)
Nov 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions src/pyprojroot/criterion.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,35 +9,50 @@
import typing
from os import PathLike as _PathLike


_PathType = typing.Union[_PathLike, str]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_CriterionType = typing.Union[
typing.Callable[[_PathType], bool],
typing.Callable[[_pathlib.Path], bool],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_PathType,
_pathlib.Path,
typing.Iterable[typing.Callable[[_PathType], bool]],
typing.Iterable[typing.Callable[[_pathlib.Path], bool]],
]

# 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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jamesmyatt jamesmyatt Nov 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

) -> typing.Callable[[_pathlib.Path], bool]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

else:

Copy link
Contributor

@jamesmyatt jamesmyatt Nov 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

criterion_collection = list(criterion) # type: ignore[arg-type]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def f(path: _pathlib.Path) -> bool:
for c in criterion:
for c in criterion_collection:
if isinstance(c, _PathLike):
if (path / c).exists():
return True
else:
if callable(c):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jamesmyatt jamesmyatt Nov 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if c(path):
return True
return False

return f


def has_file(file: _PathLike) -> typing.Callable:
def has_file(file: _PathType) -> typing.Callable[[_pathlib.Path], bool]:
"""
Check that specified file exists in path.

Expand All @@ -50,7 +65,7 @@ def f(path: _pathlib.Path) -> bool:
return f


def has_dir(file: _PathLike) -> typing.Callable:
def has_dir(file: _PathType) -> typing.Callable[[_pathlib.Path], bool]:
"""
Check that specified directory exists.

Expand All @@ -63,7 +78,7 @@ def f(path: _pathlib.Path) -> bool:
return f


def matches_glob(pat: str) -> typing.Callable:
def matches_glob(pat: str) -> typing.Callable[[_pathlib.Path], bool]:
"""
Check that glob has at least one match.
"""
Expand Down
9 changes: 6 additions & 3 deletions src/pyprojroot/here.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@

import pathlib as _pathlib
import warnings as _warnings
from os import PathLike as _PathLike
import typing

from . import criterion
from .root import find_root_with_reason


CRITERIA = [
criterion.has_file(".here"),
criterion.has_dir(".git"),
Expand All @@ -26,7 +27,7 @@
]


def get_here():
def get_here() -> typing.Tuple[_pathlib.Path, str]:
# TODO: This should only find_root once per session
start = _pathlib.Path.cwd()
path, reason = find_root_with_reason(CRITERIA, start=start)
Expand All @@ -36,7 +37,9 @@ def get_here():
# TODO: Implement set_here


def here(relative_project_path: _PathLike = "", warn_missing=False) -> _pathlib.Path:
def here(
relative_project_path: criterion._PathType = "", warn_missing: bool = False
) -> _pathlib.Path:
"""
Returns the path relative to the projects root directory.
:param relative_project_path: relative path from project root
Expand Down
19 changes: 13 additions & 6 deletions src/pyprojroot/root.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,15 @@

import pathlib as _pathlib
import typing as _typing
from os import PathLike as _PathLike

from .criterion import as_root_criterion as _as_root_criterion
from .criterion import (
as_root_criterion as _as_root_criterion,
_CriterionType,
_PathType,
)


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if start is None:
return _pathlib.Path.cwd()
if not isinstance(start, _pathlib.Path):
Expand All @@ -22,7 +25,8 @@ def as_start_path(start: _PathLike) -> _pathlib.Path:


def find_root_with_reason(
criterion, start: _PathLike = None
criterion: _CriterionType,
start: _typing.Union[None, _PathType] = None,
) -> _typing.Tuple[_pathlib.Path, str]:
"""
Find directory matching root criterion with reason.
Expand Down Expand Up @@ -51,15 +55,18 @@ def find_root_with_reason(
raise RuntimeError("Project root not found.")


def find_root(criterion, start: _PathLike = None, **kwargs) -> _pathlib.Path:
def find_root(
criterion: _CriterionType,
start: _typing.Union[None, _PathType] = None,
) -> _pathlib.Path:
"""
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

except RuntimeError as ex:
raise ex
else:
Expand Down