Skip to content

Handle only known functions in rm_rf #5627

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
Merged
merged 1 commit into from
Aug 2, 2019
Merged

Handle only known functions in rm_rf #5627

merged 1 commit into from
Aug 2, 2019

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Jul 18, 2019

Ref: #5626

blueyed
blueyed commented Jul 18, 2019
View reviewed changes
@codecov
Copy link

codecov bot commented Jul 20, 2019 •
edited
Loading

Codecov Report

Merging #5627 into master will increase coverage by 0.03%.
The diff coverage is 98.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5627      +/-   ##
==========================================
+ Coverage    96.1%   96.13%   +0.03%     
==========================================
  Files         117      117              
  Lines       25797    25822      +25     
  Branches     2495     2497       +2     
==========================================
+ Hits        24791    24825      +34     
+ Misses        701      693       -8     
+ Partials      305      304       -1
Impacted Files Coverage Δ
testing/test_tmpdir.py 98.66% <100%> (+0.13%) ⬆️
src/_pytest/pathlib.py 89.79% <95.83%> (+1.9%) ⬆️
src/_pytest/nodes.py 95.52% <0%> (-0.03%) ⬇️
src/_pytest/doctest.py 96.62% <0%> (+2.24%) ⬆️

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 dc6e7b9...9064eea. Read the comment docs.

@blueyed
Copy link
Contributor Author

blueyed commented Jul 20, 2019

@nicoddemus
Feel free to pick this up. I am on features for now.. ;)

@blueyed blueyed added type: bug problem that needs to be addressed type: regression indicates a problem that was introduced in a release which was working previously labels Jul 20, 2019
@nicoddemus
Copy link
Member

Feel free to pick this up. I am on features for now.. ;)

What do you mean, has this been fixed in features? I missed that

@blueyed
Copy link
Contributor Author

blueyed commented Jul 20, 2019

No - it's not in features yet.

@nicoddemus
Copy link
Member

Sorry I'm a little confused, when you said "I'm on features now", I understood that you were using pytest@features and this was no longer an issue for you. Did you mean something else?

@blueyed
Copy link
Contributor Author

blueyed commented Jul 20, 2019

No, I've meant that: #5626 is only an issue on master, not features (since you've merged it to master).
IIRC it is not released yet (and master was not merged into features - I was thinking about this initially, but figured it would be better to fix it on master).

@nicoddemus
Copy link
Member

Ahh got it, thanks. 👍

I'll pick this up, probably will copy pre-commit's solution given that this is probably already battle-tested.

@nicoddemus
Copy link
Member

nicoddemus commented Jul 20, 2019 •
edited
Loading

(Taking in account that your issue was really with os.open)

@nicoddemus nicoddemus self-assigned this Jul 20, 2019
@nicoddemus
Copy link
Member

@blueyed moved the internal error function to the module level and added a unittest. I think this is the best we can do, as testing the actual cases would involve a lot of work.

Please rebase/merge if you think this is good. 👍

@blueyed
Copy link
Contributor Author

blueyed commented Jul 31, 2019

@nicoddemus
Looks decent, thanks!

@RonnyPfannschmidt
Copy link
Member

looks awesome, please sort the type error on py35

@nicoddemus
Copy link
Member

Should be ready, @blueyed please squash/merge when you have a chance, thanks!

@nicoddemus
Copy link
Member

After this, I plan to port this to 4.6-maintenance, and prepare 5.1 and 4.6.5. 🎉

@nicoddemus
Copy link
Member

@blueyed let me know if you want me to squash this myself.

@blueyed
Copy link
Contributor Author

blueyed commented Aug 2, 2019

@nicoddemus
Please go ahead - I won't have time for this today.

@blueyed @nicoddemus
Improve rm_rf to handle only known functions
9064eea 
Warnings are emitted if we cannot safely remove
paths.

Fix pytest-dev#5626
@nicoddemus
Copy link
Member

Done. This should be merged as soon as CI passes.

@nicoddemus nicoddemus changed the title WIP: Fix rm_rf Handle only known functions in rm_rf Aug 2, 2019
@nicoddemus nicoddemus requested review from The-Compiler and removed request for The-Compiler August 2, 2019 11:39
@nicoddemus nicoddemus merged commit 0d3958e into pytest-dev:master Aug 2, 2019
@blueyed blueyed deleted the fix-rm_rf branch August 2, 2019 14:14
@nicoddemus nicoddemus added the needs backport applied to PRs, indicates that it should be ported to the current bug-fix branch label Aug 3, 2019
nicoddemus added a commit to nicoddemus/pytest that referenced this pull request Aug 3, 2019
@nicoddemus
Handle only known functions in rm_rf (pytest-dev#5627)
0274c08 
Backport of pytest-dev#5627

Conflicts:
- 	src/_pytest/pathlib.py

Also had to adapt:

- PermissionError into OSError with appropriate
- Change keyword-only argument to **kwargs
- Remove type annotations
@nicoddemus nicoddemus mentioned this pull request Aug 3, 2019
Merged
asottile added a commit that referenced this pull request Aug 5, 2019
@asottile
Merge pull request #5691 from nicoddemus/backport-5627
a19ae2a 
[4.6] Handle only known functions in rm_rf (#5627)
@asottile asottile added backported and removed needs backport applied to PRs, indicates that it should be ported to the current bug-fix branch labels Oct 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RonnyPfannschmidt RonnyPfannschmidt RonnyPfannschmidt approved these changes

Assignees

@nicoddemus nicoddemus

Labels
type: bug problem that needs to be addressed type: regression indicates a problem that was introduced in a release which was working previously
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@blueyed @nicoddemus @RonnyPfannschmidt @asottile