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

Some py.path -> pathlib conversions #7619

Merged
merged 4 commits into from
Aug 7, 2020

Conversation

bluetech
Copy link
Member

@bluetech bluetech commented Aug 4, 2020

The first commit adds analogues of py.path.local's bestrelpath and common functions to _pytest.pathlib.

The second commit converts _pytest.config.findpaths to use pathlib.

The third commit coverts some random places to pathlib.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Awesome work @bluetech! I've left a few comments, let me know what you think! 👍

src/_pytest/pathlib.py Show resolved Hide resolved
src/_pytest/config/findpaths.py Outdated Show resolved Hide resolved
src/_pytest/config/findpaths.py Outdated Show resolved Hide resolved
src/_pytest/config/findpaths.py Outdated Show resolved Hide resolved
src/_pytest/config/findpaths.py Outdated Show resolved Hide resolved
src/_pytest/config/findpaths.py Outdated Show resolved Hide resolved
@bluetech bluetech force-pushed the py-to-pathlib branch 4 times, most recently from dd12f05 to 7ddf479 Compare August 5, 2020 15:49
)
p = Path(entry.path)

parents = p.parents
Copy link
Member

Choose a reason for hiding this comment

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

Gotta love Path.parents. 😁

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Great work! Left two minor comments, feel free to address them or not, up to you! 👍

return path.parent

def safe_exists(path: Path) -> bool:
# On Python<=3.7, this can throw on paths that contain characters
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should use sys.version_info here as guard, so safe_exists can be dropped once only Python 3.8+ is supported?

if sys.version_info[:2] <= (3, 7):
    def safe_exists(path: Path) -> bool:
        # On Python<=3.7, this can throw on paths that contain characters
        # unrepresentable at the OS level.
        try:
            return path.exists()
        except OSError:
            return False
else:
    def safe_exists(path: Path) -> bool:
        return path.exists()

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, will change it.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW. The reason this can happen at all is pretty funky.

pytest determines the rootdir by looking at all non-option arguments and finding the common ancestor etc.

But because this code runs before plugins are loaded, any plugin argument not prefixed by a -, such as test*bla in pytest --tx test*bla (as opposed to if the --tx=test*bla form was used) gets interpreted as a possible path. If it is actually a valid path then you'd have one seriously confused user...

I looked into fixing it but seems not really possible.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it is not simple due to how options are parsed. 😕

testing/test_config.py Show resolved Hide resolved
An equivalent for these py.path.local functions is needed for some
upcoming py.path -> pathlib conversions.
Didn't call it absolute or absolute_path to avoid conflicts with
possible variable names.

Didn't call it abspath to avoid confusion with os.path.abspath.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants