-
Notifications
You must be signed in to change notification settings - Fork 304
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
Fix the non-writable path deletion error #670
Fix the non-writable path deletion error #670
Conversation
Thanks for submitting your first pull request! You are awesome! 🤗 |
Codecov Report
@@ Coverage Diff @@
## main #670 +/- ##
==========================================
+ Coverage 77.88% 78.38% +0.49%
==========================================
Files 110 110
Lines 10405 10696 +291
Branches 1400 1441 +41
==========================================
+ Hits 8104 8384 +280
- Misses 1911 1916 +5
- Partials 390 396 +6
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
Kicking CI |
Kicking CI one more time after #669 |
Kicking to pick up windows skips |
CI failures are unrelated and captured in #677. Thanks again @vkaidalov! |
Hi everyone!
I noticed that I got the "Delete Failed: Unhandled error" message in the error dialog when I tried to delete either a read-only file or a read-only directory. In contrast, the message was "Permission denied" in JupyterLab 1.x, so I decided to take a look at the code.
As the result, I found out that this issue had already been identified in the comments. Given that the "easier to ask for forgiveness than permission" approach cannot be used there because of send2trash's errors ambiguity, I propose to "look before you leap" to fix this issue.
Could you please review the PR? It should make the server return 403 instead of 500 and also make JupyterLab show the "Permission denied" message instead of the "Unhandled error" text. I've also moved the permissions check code to a separate new method.