-
Notifications
You must be signed in to change notification settings - Fork 16
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
d7595da
02e742d
71cb1f0
decaf66
84ad46a
428b331
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 |
---|---|---|
|
@@ -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, | ||
|
@@ -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: | ||
""" | ||
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 | ||
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. 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() | ||
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. (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. 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 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 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. 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. 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. Totally understand. I appreciate your time as well. Will attempt to finalize this tomorrow. |
||
return Path.cwd() | ||
|
||
|
||
def find_root_with_reason( | ||
|
@@ -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" | ||
|
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.
I'd say that the 1 line summary is along the lines of: