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

Jump to bottom

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
Open

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

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d7595da
siplified the as_start_path function and remove the #todo as it it ca…
jasonbrackman Mar 20, 2023
02e742d
Added an expanduser() before returning path to ensure that any input …
jasonbrackman Mar 22, 2023
71cb1f0
removed space from end of lines.
jasonbrackman Mar 22, 2023
decaf66
- add .resolve() to handle the '.' and '..' normalization for the Pat…
jasonbrackman Mar 22, 2023
84ad46a
removed white space
jasonbrackman Mar 22, 2023
428b331
Looks like I need to have the tests run from pyprojroot. I'll need t…
jasonbrackman Mar 22, 2023
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
12 changes: 4 additions & 8 deletions src/pyprojroot/root.py
View file
Open in desktop
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,9 @@
)


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:
"""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)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure that we need to do something like resolve or expanduser. So the TODO still stands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

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'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.

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if you call find_root(criterion, start=".") or find_root(criterion, start="~/project")?

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

  1. I had to convert the tilde (is that what you expect?)
  2. 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

Copy link
Contributor

@jamesmyatt jamesmyatt Mar 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 "."!



def find_root_with_reason(
Expand Down