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

Keep using jupytext.json for configuring the extensions #1186

Merged
merged 1 commit into from
Dec 3, 2023

Conversation

mwouts
Copy link
Owner

@mwouts mwouts commented Dec 2, 2023

Closes #1185

Copy link

github-actions bot commented Dec 2, 2023

Thank you for making this pull request.

Did you know? You can try it on Binder: Binder:lab or Binder:notebook.

Also, the version of Jupytext developed in this PR can be installed with pip:

pip install git+https://github.com/mwouts/jupytext.git@rename_jupyterlab_jupytext_to_jupytext

(this requires nodejs, see more at Developing Jupytext)

@mwouts
Copy link
Owner Author

mwouts commented Dec 2, 2023

@mahendrapaipuri I would prefer to keep the setting files unchanged, to avoid getting in trouble with e.g. older configuration files left behind. Can you have a quick look at this PR and let me know what you think? Thanks!

Copy link

codecov bot commented Dec 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7223622) 97.73% compared to head (28665a8) 97.73%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1186   +/-   ##
=======================================
  Coverage   97.73%   97.73%           
=======================================
  Files          29       29           
  Lines        4456     4456           
=======================================
  Hits         4355     4355           
  Misses        101      101           
Flag Coverage Δ
external 75.17% <ø> (ø)
functional 88.56% <ø> (ø)
integration 77.31% <ø> (ø)
unit 66.56% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mwouts mwouts force-pushed the rename_jupyterlab_jupytext_to_jupytext branch from e4b9d30 to f19a18c Compare December 2, 2023 20:35
@mahendrapaipuri
Copy link
Contributor

When we reorganized the repository, we made a decision to split core jupytext and jupyterlab extension. My opinion is that we should try to keep it that way for better maintainability.

But the issue you raised in #1185 is a valid one. What we can do is add a build hook that cleans up the previous extension config file. As you mentioned in the issue, the error is non fatal. Jupyter server is simply trying to load a non existent extension. So, this is not a real blocker for the users. Is this sort of solution you are willing to accept? If so, I can try to draft a quick PR.

@mahendrapaipuri
Copy link
Contributor

I guess there is even a simpler solution to this. We can rename jupyterlab-jupytext.json file to jupytext.json. This overwrites the legacy config file (if present in the env) from previous versions and we get rid of these warnings. Could you give it a try?

@mwouts
Copy link
Owner Author

mwouts commented Dec 2, 2023

Oh yes, that would be a nice way to do it! I will give it a try tomorrow. Thanks!

@mwouts mwouts force-pushed the rename_jupyterlab_jupytext_to_jupytext branch from f19a18c to 28665a8 Compare December 2, 2023 23:04
@mwouts mwouts changed the title Keep jupytext as the name of the server extension Keep using jupytext.json for configuring the extensions Dec 2, 2023
@mwouts
Copy link
Owner Author

mwouts commented Dec 3, 2023

Hey @mahendrapaipuri , just to double check, is this new change what you suggested above ? Thanks !

@mahendrapaipuri
Copy link
Contributor

@mwouts Exactly. This will overwrite existing config file (if any) in the current env.

@mwouts mwouts merged commit 9273353 into main Dec 3, 2023
31 checks passed
@mwouts mwouts deleted the rename_jupyterlab_jupytext_to_jupytext branch December 3, 2023 17:18
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.

Extension fails to load in ServerApp
2 participants