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

Use PurePath directly instead of os.path.sep in rewrite.py #10078

Merged
merged 1 commit into from Jun 27, 2022

Conversation

nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Jun 26, 2022

Given we are already creating a PurePath, just pass the parts directly to it.

This avoids using os.path.sep, that although is an official API, seems not to be available in all systems.

Fix #9791

@nicoddemus nicoddemus added the needs backport applied to PRs, indicates that it should be ported to the current bug-fix branch label Jun 26, 2022
@nicoddemus
Copy link
Member Author

The ubuntu-pypy3 is unrelated and can be ignored for now.

@The-Compiler
Copy link
Member

FWIW this is official API: https://docs.python.org/3/library/os.html#os.sep

Also available via os.path.

I wonder, since we are changing this anyways, wouldn't something like PurePath(*parts).with_suffix(".py") be more idiomatic?

@nicoddemus
Copy link
Member Author

nicoddemus commented Jun 26, 2022

Also available via os.path.

Oh TIL. Then I'm not sure what the problem in #9791 is. 🤔

We could use PurePath (I definitely prefer that in general) but perhaps here we should be more conservative and use os.path (which is faster AFAIK)?

EDIT: I remembered now that the source of os.path being faster was a video/post from @asottile.

@The-Compiler
Copy link
Member

The-Compiler commented Jun 26, 2022

We do already use PurePath though? Also not really a fan of doing premature optimization before any profiling for this specific case.

Given we are already creating a `PurePath`, just pass the parts directly to it.

This avoids using `os.path.sep`, that although is an official API, seems not to be available in all systems.

Fix pytest-dev#9791
@nicoddemus nicoddemus changed the title Fix incorrect usage of os.path.sep in rewrite.py Use PurePath directly instead of os.path.sep in rewrite.py Jun 27, 2022
@nicoddemus
Copy link
Member Author

You folks are right, updated, thanks!

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

@nicoddemus nicoddemus merged commit da9a2b5 into pytest-dev:main Jun 27, 2022
@nicoddemus nicoddemus deleted the pathsep-9791 branch June 27, 2022 12:58
@nicoddemus nicoddemus added backport 7.1.x and removed needs backport applied to PRs, indicates that it should be ported to the current bug-fix branch labels Jun 27, 2022
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.

Error when running debugging using pytest in IDE with geopandas
4 participants