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.

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

simplified the as_start_path function and remove the #todo as it it ca… #61

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
23 changes: 14 additions & 9 deletions src/pyprojroot/root.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"""

from pathlib import Path
from typing import Union, Tuple
from typing import Union, Tuple, Optional

from .criterion import (
as_root_criterion as _as_root_criterion,
Expand All @@ -15,13 +15,19 @@
)


def as_start_path(start: Union[None, _PathType]) -> Path:
if start is None:
return Path.cwd()
if not isinstance(start, Path):
start = Path(start)
# TODO: consider `start = start.resolve()`
return start
def as_start_path(start: Optional[_PathType]) -> 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'd say that the 1 line summary is along the lines of:

Convert path argument into normalised Path object.

Returns a Path object based on the current working directory or the optional input
provided. If the input `start` parameter contains the '~' character, it will be
Copy link
Contributor

Choose a reason for hiding this comment

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

Also need to normalise parts like "." and "..".

expanded to the home directory before being returned.

:param start: Optional[str or Path], the path to start from. Defaults to None
which sets the starting path to the current working directory.
:return: Path, the Path object based on the starting path.
"""
if start is not None:
return Path(start).expanduser()
Copy link
Contributor

Choose a reason for hiding this comment

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

(minor) I'm not sure it's worth converting an existing Path object into a new one. But I'm also not sure there's any harm.

Copy link
Contributor Author

@jasonbrackman jasonbrackman Mar 22, 2023

Choose a reason for hiding this comment

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

I have pushed to the change some tests, however, I'm not sure if I've set them up properly. I'm not sure of the expectation here.

Let me know if I'm missing any tests / updates needed.

Also I did remove the isinstance as there wasn't any harm. Let me know if its still more readable for you if the check is there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I'll take a look over the next few days.

Sorry if my comments have been a bit terse---I haven't had the time or energy for more---but I appreciate you trying to help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally understand. I appreciate your time as well. Will attempt to finalize this tomorrow.

return Path.cwd()


def find_root_with_reason(
Expand All @@ -39,7 +45,6 @@ def find_root_with_reason(
# Prepare inputs
criterion = _as_root_criterion(criterion)
start = as_start_path(start)

# Check start
if start.is_dir() and criterion(start):
return start, "Pass"
Expand Down