-
Notifications
You must be signed in to change notification settings - Fork 308
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 rename_file and delete_file to handle hidden files properly #1073
Fix rename_file and delete_file to handle hidden files properly #1073
Conversation
Thanks for submitting your first pull request! You are awesome! 🤗 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1073 +/- ##
==========================================
- Coverage 75.69% 75.69% -0.01%
==========================================
Files 63 63
Lines 8222 8226 +4
Branches 1634 1636 +2
==========================================
+ Hits 6224 6227 +3
- Misses 1572 1574 +2
+ Partials 426 425 -1 ☔ View full report in Codecov by Sentry. |
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.
Thank you!
@meeseeksdev please backport to 1.x |
…andle hidden files properly
…andle hidden files properly) (#1075) Co-authored-by: Satoshi Yazawa <[email protected]>
In working on the Pull Request jupyter/notebook#6609 , I found that the tests for hidden files was broken, so that I fixed the tests and found more problems to fix.
Some mistakes regarding the tests are as follows (for example, see
test_400
intests/services/contents/test_manager.py
https://github.com/jupyter-server/jupyter_server/blob/main/tests/services/contents/test_manager.py#L329-L342 ):try ... except
inwith pytest.raises(HTTPError)
, but actually the exception is raised by thecm.new
before thetry
and never performs therename_file
.old_path
ornew_path
is set to the return value ofcm._get_os_path()
(it seems an absolute path), but I assume thatrename_file
anddelete_file
expects relative paths.When I fixed the test, I found the following new problems:
AsyncFileContentsManager
rename_file
anddelete_file
, it is not checked if a path is a hidden file or not.FileContentsManager
delete_file
, the existence check has been made before determining whether the file is hidden.(This looks like the hidden check should be done first, as seen in rename_file).So I also fixed these problems.