simplified the as_start_path function and remove the #todo as it it ca… #61
Conversation
…n still be used by the callers if required.
src/pyprojroot/root.py
Outdated
| return start | ||
| def as_start_path(start: Optional[_PathType]) -> Path: | ||
| """A pathlib.Path object based on the common working directory or the optional input provided.""" | ||
| return Path.cwd() if start is None else Path(start) |
There was a problem hiding this comment.
I'm pretty sure that we need to do something like resolve or expanduser. So the TODO still stands.
There was a problem hiding this comment.
Hi James,
I was wondering what the usecase was. The Path returned can have the caller call resolve(). Is there a reason for doing this step at this time?
There was a problem hiding this comment.
I'm also realizing I wrote that it is using the 'common working directory -- but its the 'current working directory'. Would also need to fix that depending on how this conversation goes :). I'm interested to understand the resolve aspect though.
There was a problem hiding this comment.
What happens if you call find_root(criterion, start=".") or find_root(criterion, start="~/project")?
There was a problem hiding this comment.
I asked the above question by creating a script called examination and placed it here:
/usr/local/bin/python3 /Users/jasonbrackman/PycharmProjects/pyprojroot/tests/examination.py
- I had to convert the tilde (is that what you expect?)
- I noticed that the projects gets clipped, so added a third case. I'm assuming this is not what you expect?
Demo Code:
import os
from pathlib import Path
from src.pyprojroot import has_dir, find_root
criteria = has_dir(Path('/tmp'))
test01 = find_root(criteria, start='.')
test02 = find_root(criteria, start=r'~/projects'.replace("~", os.path.expanduser('~')))
test03 = find_root(criteria, start=r'~/projects/weird'.replace("~", os.path.expanduser('~')))
for test in (test01, test02, test03):
print(f'Returned: {test!s:30} >> Resolved: {test.resolve()!s}')
Resulted in:
Returned: . >> Resolved: /Users/jasonbrackman/PycharmProjects/pyprojroot/tests
Returned: /Users/jasonbrackman >> Resolved: /Users/jasonbrackman
Returned: /Users/jasonbrackman/projects >> Resolved: /Users/jasonbrackman/projects
There was a problem hiding this comment.
The answer will of course depend on the criteria and the files/directories present, but I don't think it should the user to expand or resolve the input in advance. Otherwise we would also require the user to pass a Path object rather than anything PathLike.
The biggest issue is with start="." since the current implementation will not look any further up the heirarchy than ".". For "~", it's unlikely that anyone will look for a root that is in a parent of the "~" directory.
There was a problem hiding this comment.
E.g.
/user/projects/
- big_analysis/
- .here <-- marker file
- notebooks/
- Untitled.ipynb
Using v0.3.0, calling find_root(".here", start=".") from the notebooks directory will raise RuntimeError rather than "/user/projects/big_analysis".
There was a problem hiding this comment.
Updated the review to take advantage of .expanduser() before return the path. Here is my input/output:
import os
from src.pyprojroot import find_root
# Set the working directory to start from
os.chdir(r'/Users/jasonbrackman/projects/big_analysis')
test01 = find_root('.here', start='.')
test02 = find_root('.here', start=r'~/projects')
for test in (test01, test02):
print(f'Returned: {test!s:30} >> Resolved: {test.resolve()!s}')
Produces:
Returned: . >> Resolved: /Users/jasonbrackman/projects/big_analysis
Returned: /Users/jasonbrackman >> Resolved: /Users/jasonbrackman
There was a problem hiding this comment.
You haven't addressed normalising the . and .. parts. find_root('.here', start='.') only works for you because there's a .here file in that directory. If you call it from a sub-directory, then it will raise a RuntimeError. Similarly if you run find_root('.here', start='..'), then it returns "."!
…containing the home directory is expanded.
src/pyprojroot/root.py
Outdated
| # TODO: consider `start = start.resolve()` | ||
| return start | ||
| def as_start_path(start: Optional[_PathType]) -> Path: | ||
| """ |
There was a problem hiding this comment.
I'd say that the 1 line summary is along the lines of:
Convert path argument into normalised Path object.
src/pyprojroot/root.py
Outdated
| 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.
Also need to normalise parts like "." and "..".
src/pyprojroot/root.py
Outdated
| :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.
(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.
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.
There was a problem hiding this comment.
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.
Totally understand. I appreciate your time as well. Will attempt to finalize this tomorrow.
…h return. - added test_root.py
… figure out how to do this locally.
| """Convert path argument into normalised Path object.""" | ||
| if start is not None: | ||
| return Path(start).expanduser().resolve() | ||
| return Path.cwd().resolve() |
There was a problem hiding this comment.
I think resolve includes expanduser and cwd is always an absolute path. So these can be simplified.
…n still be used by the callers if required.