Skip to content

Use builtin pathlib on Python 3 #5022

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
Closed
wants to merge 1 commit into from
Closed

Use builtin pathlib on Python 3 #5022

wants to merge 1 commit into from

Conversation

@nicoddemus
Copy link
Member

Fix #5017

@codecov
Copy link

codecov bot commented Apr 2, 2019 •
edited
Loading

Codecov Report

Merging #5022 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5022      +/-   ##
==========================================
+ Coverage   95.98%   95.98%   +<.01%     
==========================================
  Files         114      114              
  Lines       25496    25505       +9     
  Branches     2474     2475       +1     
==========================================
+ Hits        24473    24482       +9     
  Misses        718      718              
  Partials      305      305
Impacted Files Coverage Δ
testing/test_cacheprovider.py 99.74% <100%> (ø) ⬆️
src/_pytest/tmpdir.py 100% <100%> (ø) ⬆️
src/_pytest/cacheprovider.py 97.84% <100%> (+0.03%) ⬆️
testing/test_reports.py 100% <100%> (ø) ⬆️
src/_pytest/pathlib.py 88.82% <100%> (-0.13%) ⬇️
testing/test_tmpdir.py 98.33% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be84ba8...e4940c1. Read the comment docs.

@RonnyPfannschmidt
Copy link
Member

This is a breaking change since it drops supported cappabilities in python 3.5, it requires a major release

@nicoddemus
Copy link
Member Author

@RonnyPfannschmidt oh you are absolutely right, thanks for catching that. I will mark this as WIP and schedule it for 5.0. 👍

@nicoddemus nicoddemus changed the title Use builtin pathlib on Python 3 [WIP] Use builtin pathlib on Python 3 Apr 2, 2019
@nicoddemus nicoddemus added this to the 5.0 milestone Apr 2, 2019
@blueyed
Copy link
Contributor

blueyed commented Apr 2, 2019

Related maybe: #4521

@nicoddemus
Copy link
Member Author

Related maybe: #4521

Sorry, what's the relation between this and #4521? 🤔

@blueyed
Copy link
Contributor

blueyed commented Apr 3, 2019

Sorry, I've meant #4721 (in terms of conflicts mostly I guess).

@nicoddemus nicoddemus force-pushed the drop-pathlib2-on-py3 branch from 856c99e to 0c803cb Compare June 5, 2019 01:42
@nicoddemus nicoddemus changed the title [WIP] Use builtin pathlib on Python 3 Use builtin pathlib on Python 3 Jun 5, 2019
@nicoddemus nicoddemus changed the base branch from features to master June 5, 2019 01:42
@nicoddemus
Copy link
Member Author

Updated/rebased. 👍

@nicoddemus nicoddemus force-pushed the drop-pathlib2-on-py3 branch 2 times, most recently from 4109b1c to e503c8c Compare June 5, 2019 01:46
blueyed
blueyed reviewed Jun 5, 2019
View reviewed changes
@nicoddemus nicoddemus force-pushed the drop-pathlib2-on-py3 branch 2 times, most recently from e2b5448 to 0542bef Compare June 5, 2019 13:15
@nicoddemus nicoddemus force-pushed the drop-pathlib2-on-py3 branch from 0542bef to e4940c1 Compare June 5, 2019 21:35
@nicoddemus
Copy link
Member Author

Fixed finally. 👍

Reviews are welcome.

asottile
asottile reviewed Jun 8, 2019
View reviewed changes
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.

otherwise looks good!

src/_pytest/cacheprovider.py
res = self._cachedir.joinpath("d", name)
res.mkdir(exist_ok=True, parents=True)
if not res.is_dir():
os.makedirs(str(res))
Copy link
Member

@asottile asottile Jun 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

os.makedirs(..., exist_ok=True) I believe is less prone to race conditions

@RonnyPfannschmidt
Copy link
Member

i still think we should rather limit python versions than land this breaking api change

@asottile
Copy link
Member

asottile commented Jun 8, 2019

i still think we should rather limit python versions than land this breaking api change

like drop python3.5? :D

@RonnyPfannschmidt
Copy link
Member

@asottile yep 🙈 🙊 🙉

@nicoddemus
Copy link
Member Author

like drop python3.5? :D

You guys really think this is a good idea? I mean, we had not released long term plans regarding Python 3.5...

@nicoddemus
Copy link
Member Author

From https://www.python.org/dev/peps/pep-0478/:

However, generally speaking, we expect to release Python 3.5.8 around September 2019.

@asottile
Copy link
Member

I suspect we can't drop python3.5 just yet, given debian stable and ubuntu xenial are very popular still

as much as it would be nice to start using f-strings 😆

@RonnyPfannschmidt
Copy link
Member

@nicoddemus i'd much rather document that the pathlib on python 3.4 is pathlib2 than breaking the api and massiviely degrading cappabilities

@nicoddemus
Copy link
Member Author

@nicoddemus i'd much rather document that the pathlib on python 3.4 is pathlib2 than breaking the api and massiviely degrading cappabilities

This is already documented though: https://docs.pytest.org/en/latest/reference.html#_pytest.tmpdir.tmp_path.

Should we close this and #5017 as "won't fix then"?

@nicoddemus
Copy link
Member Author

@RonnyPfannschmidt @asottile gentle ping.

@RonnyPfannschmidt
Copy link
Member

im in favor of close + wontfix

@nicoddemus nicoddemus closed this Jun 13, 2019
@nicoddemus nicoddemus deleted the drop-pathlib2-on-py3 branch June 13, 2019 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RonnyPfannschmidt RonnyPfannschmidt RonnyPfannschmidt left review comments

@asottile asottile asottile left review comments

+1 more reviewer

@blueyed blueyed blueyed left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

5.0

Development

Successfully merging this pull request may close these issues.

Pathlib compat fails under Python 3.5

4 participants

@nicoddemus @RonnyPfannschmidt @blueyed @asottile