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

Enable extensions to control the file_to_run #415

Merged
merged 1 commit into from
Feb 18, 2021
Merged

Enable extensions to control the file_to_run #415

merged 1 commit into from
Feb 18, 2021

Conversation

afshin
Copy link
Contributor

@afshin afshin commented Feb 17, 2021

In classic notebook, users could open a file directly, e.g.:

jupyter notebook mynotebook.ipynb

The server would start at the parent directory of mynotebook.ipynb and open the browser at /<base_url>/notebook/mynotebook.ipynb. Alternatively, users could get the same behavior using the --NotebookApp.file_to_run trait.

Jupyter server still has the file_to_run trait, but the <base_url>/notebook prefix is hard-coded, even though it doesn't make sense for server anymore. Further, extensions like JupyterLab cannot easily override this prefix.

This PR adds a new configurable trait, file_url_prefix, to both ServerApp and ExtensionApp. It changes the URL prefix where the server starts when a file_to_run is given. ExtensionApp can define their own file_url_prefix default value to control this trait when they launch a server. This should fix jupyterlab/jupyterlab#8959 (comment).

This PR also addresses some brittleness around corner cases where file_to_run and root_dir are both given and verified by the new unit tests added.

@codecov-io
Copy link

codecov-io commented Feb 17, 2021

Codecov Report

Merging #415 (1bbcbcb) into master (01f0f43) will increase coverage by 0.15%.
The diff coverage is 69.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #415      +/-   ##
==========================================
+ Coverage   69.94%   70.09%   +0.15%     
==========================================
  Files          57       57              
  Lines        6311     6340      +29     
  Branches      847      850       +3     
==========================================
+ Hits         4414     4444      +30     
+ Misses       1593     1591       -2     
- Partials      304      305       +1     
Impacted Files Coverage Δ
jupyter_server/serverapp.py 65.85% <68.85%> (+1.28%) ⬆️
jupyter_server/extension/application.py 72.44% <100.00%> (+0.14%) ⬆️
jupyter_server/pytest_plugin.py 91.52% <100.00%> (-0.10%) ⬇️

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 01f0f43...1bbcbcb. Read the comment docs.

Copy link
Member

@Zsailer Zsailer left a comment

Choose a reason for hiding this comment

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

Thanks, @afshin!

@blink1073 blink1073 merged commit 7f63dde into jupyter-server:master Feb 18, 2021
@echarles
Copy link
Member

Great job @afshin. I am a bit late to the party... is the trait file_url_prefix documented for ExtensionApp authors?

@jtpio
Copy link
Member

jtpio commented Feb 19, 2021

is the trait file_url_prefix documented for ExtensionApp authors?

Maybe it could also be useful to document how to create a redirect handler using the file_url_prefix, to make it possible to handle files differently if needed.

Thinking of nbclassic and jupyterlab-classic that would open a file editor (/edit) or notebook page (/notebook) depending on the file extension.

@Zsailer
Copy link
Member

Zsailer commented Feb 19, 2021

@echarles, see #420.

@jtpio, let's add this in a separate docs PR from #420, since this is a bit more advanced.

try:
browser = webbrowser.get(self.browser or None)
except webbrowser.Error as e:
self.log.warning(_('No web browser found: %s.') % e)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,

When I start jupyter from CUI, the program will stop with the following error.

...
  File "/root/.pyenv/versions/3.7.3/lib/python3.7/site-packages/jupyter_server/serverapp.py", line 2020, in launch_browser
    self.log.warning(_('No web browser found: %s.') % e)
UnboundLocalError: local variable '_' referenced before assignment

As shown in the error message, by removing the _ from the relevant part, jupyter can be executed successfully.

Looking at this script, it seems that underscores _ are inserted in self.log.info() and so on.
I am not familiar with this notation, so I would like to confirm it.

Thanks in advance,

Copy link
Member

Choose a reason for hiding this comment

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

Notebook server and Jupyter Server (due to its derivation from the former) define _ to be the method trans.gettext such that any strings wrapped in _() are candidates for translation lookups. However, this precludes the common use of _ to represent an ignored value in the same method - as is the case here (a few lines below):

        assembled_url, _ = self._prepare_browser_open()

Seems like we should no longer return open_file from _prepare_browser_open() (since this is the only call to it) or adjust the name for the ignored value.

I don't know the history of using _ for trans.gettext but using something like _i18n instead seems more clear while not being too intrusive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification!

Now I see how _ is used here, but as it is pointed out, it is neither clear nor safe usage, I guess...
This is a common way to represent ignored variables with _ in python, so I agree with @kevin-bates's suggestion!

Copy link
Member

@kevin-bates kevin-bates Feb 22, 2021

Choose a reason for hiding this comment

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

I've opened #424 to help avoid this in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open a file from command line
8 participants