Skip to content

Use builtin pathlib on Python 3 - #5022

Closed
nicoddemus wants to merge 1 commit into
pytest-dev:masterfrom
nicoddemus:drop-pathlib2-on-py3
Closed

Use builtin pathlib on Python 3#5022
nicoddemus wants to merge 1 commit into
pytest-dev:masterfrom
nicoddemus:drop-pathlib2-on-py3

Conversation

@nicoddemus

@nicoddemus nicoddemus commented Apr 2, 2019

Copy link
Copy Markdown
Member

Fix #5017

@codecov

codecov Bot commented Apr 2, 2019 •
edited
Loading

Copy link
Copy Markdown

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

RonnyPfannschmidt commented Apr 2, 2019

Copy link
Copy Markdown
Member

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

@nicoddemus

nicoddemus commented Apr 2, 2019

Copy link
Copy Markdown
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

blueyed commented Apr 2, 2019

Copy link
Copy Markdown
Contributor

Related maybe: #4521

@nicoddemus

nicoddemus commented Apr 3, 2019

Copy link
Copy Markdown
Member Author

Related maybe: #4521

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

@blueyed

blueyed commented Apr 3, 2019

Copy link
Copy Markdown
Contributor

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

nicoddemus commented Jun 5, 2019

Copy link
Copy Markdown
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
Comment thread src/_pytest/cacheprovider.py Outdated
@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

nicoddemus commented Jun 6, 2019

Copy link
Copy Markdown
Member Author

Fixed finally. 👍

Reviews are welcome.

asottile
asottile reviewed Jun 8, 2019
View reviewed changes

@asottile asottile left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

otherwise looks good!

Comment thread 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))

@asottile asottile Jun 8, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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

RonnyPfannschmidt commented Jun 8, 2019

Copy link
Copy Markdown
Member

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

@asottile

asottile commented Jun 8, 2019

Copy link
Copy Markdown
Member

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

like drop python3.5? :D

@RonnyPfannschmidt

RonnyPfannschmidt commented Jun 8, 2019

Copy link
Copy Markdown
Member

@asottile yep 🙈 🙊 🙉

@nicoddemus

nicoddemus commented Jun 11, 2019

Copy link
Copy Markdown
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

nicoddemus commented Jun 11, 2019

Copy link
Copy Markdown
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

asottile commented Jun 11, 2019

Copy link
Copy Markdown
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

RonnyPfannschmidt commented Jun 11, 2019

Copy link
Copy Markdown
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

nicoddemus commented Jun 11, 2019

Copy link
Copy Markdown
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

nicoddemus commented Jun 12, 2019

Copy link
Copy Markdown
Member Author

@RonnyPfannschmidt @asottile gentle ping.

@RonnyPfannschmidt

RonnyPfannschmidt commented Jun 12, 2019

Copy link
Copy Markdown
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