Skip to content
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

Do not check if hidden if hidden files are allowed #1224

Closed
vidartf opened this issue Mar 2, 2023 · 2 comments · Fixed by #1226
Closed

Do not check if hidden if hidden files are allowed #1224

vidartf opened this issue Mar 2, 2023 · 2 comments · Fixed by #1226
Labels

Comments

@vidartf
Copy link
Member

vidartf commented Mar 2, 2023

Description

Currently, even if allow_hidden is set to true, we still call the potentially expensive check for each and every file. The clauses in the conditional should be switched so that it short-circuits on the setting.

Reproduce

  1. Go to '...'
  2. Click on '...'
  3. Scroll down to '...'
  4. See error '...'

Expected behavior

Context

  • Operating System and version:
  • Browser and version:
  • Jupyter Server version:
Troubleshoot Output
Paste the output from running `jupyter troubleshoot` from the command line here.
You may want to sanitize the paths in the output.
Command Line Output
Paste the output from your command line running `jupyter lab` here, use `--debug` if possible.
Browser Output
Paste the output from your browser Javascript console here, if applicable.
@vidartf vidartf added the bug label Mar 2, 2023
@vidartf
Copy link
Member Author

vidartf commented Mar 2, 2023

Also, does the async file manager not need to await the is_hidden calls?

@kevin-bates
Copy link
Member

kevin-bates commented Mar 2, 2023

Also, does the async file manager not need to await the is_hidden calls?

There are two is_hidden methods in play here. One takes two parameters and is a synchronous library function from jupyter_core, and the other is a class method defined on the respective contents manager classes, with AsyncFileContentsManager's is_hidden() being async. It looks like the references in AFCM are to the library function.

Frankly, I don't think AFCM's is_hidden, dir_exists, and file_exists don't need to exist (and I suspect there may be others like that). If these were to be removed, the handlers will also need to be updated since they currently ensure_async on these.

vidartf-jpmc pushed a commit to vidartf-jpmc/jupyter-fs that referenced this issue May 17, 2023
Also addresses
jupyter-server/jupyter_server#1224 for our
implementation.

Also does:
- Improves tests by using an actual jupyter server instance instead of
  calling CM methods directly (gains config + handler logic)
- Moves conftest to root, as this is needed by pytest for naming plugins
- FSManager init calls super: this enables the traitlets config system
timkpaine pushed a commit to jpmorganchase/jupyter-fs that referenced this issue May 19, 2023
Also addresses
jupyter-server/jupyter_server#1224 for our
implementation.

Also does:
- Improves tests by using an actual jupyter server instance instead of
  calling CM methods directly (gains config + handler logic)
- Moves conftest to root, as this is needed by pytest for naming plugins
- FSManager init calls super: this enables the traitlets config system
vidartf added a commit to jpmorganchase/jupyter-fs that referenced this issue May 24, 2023
Also addresses
jupyter-server/jupyter_server#1224 for our
implementation.

Also does:
- Improves tests by using an actual jupyter server instance instead of
  calling CM methods directly (gains config + handler logic)
- Moves conftest to root, as this is needed by pytest for naming plugins
- FSManager init calls super: this enables the traitlets config system
vidartf added a commit to jpmorganchase/jupyter-fs that referenced this issue Jun 6, 2023
Also addresses
jupyter-server/jupyter_server#1224 for our
implementation.

Also does:
- Improves tests by using an actual jupyter server instance instead of
  calling CM methods directly (gains config + handler logic)
- Moves conftest to root, as this is needed by pytest for naming plugins
- FSManager init calls super: this enables the traitlets config system
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants