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

Normalize jupytext config options names #754

Merged
merged 18 commits into from
Mar 18, 2021

Conversation

mwouts
Copy link
Owner

@mwouts mwouts commented Mar 15, 2021

This PR goes a bit further than #753 and normalizes the names of the options in the jupytext.toml file, to match the metadata seen in the Jupytext notebooks. In particular:

  • default_jupytext_formats becomes formats
  • default_notebook_metadata_filter becomes notebook_metadata_filter
  • default_cell_metadata_filter becomes cell_metadata_filter
  • default_cell_markers becomes cell_markers

Previous option names are still supported but will cause a DeprecationWarning.

@mwouts
Copy link
Owner Author

mwouts commented Mar 15, 2021

I'll be happy to have some user feedback on this PR...

@chrisjsewell , @JohnPaton , @matthew-brett , @grst do you think that renaming e.g. default_jupytext_formats in the Jupytext configuration file (e.g. jupytext.toml) to simply formats, like in the notebook themselves, makes sense? Do we agree that examples config files in the tentative version are neater?

Installing the dev version can be done with

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

(prefix the above with BUILD_JUPYTERLAB_EXTENSION=1 if you want to build the extension for JupyterLab, but that will require node)

NB: Using the previous names is still an option, but using them will cause a DeprecationWarning. Is that OK, or too much? Should I wait for a few release before activating the deprecation warning? Advice is welcome! Thanks!

@JohnPaton
Copy link
Contributor

I think this is neater, and also that the deprecation warning is fine, as long as the old names stay supported for several versions to give people time to switch

@grst
Copy link
Contributor

grst commented Mar 16, 2021

Hi Marc,

I really like this change, I was always struggling to remember how this setting was called.

While we are at it, wouldn't it be more natural to use a TOML list directly instead of splitting a string? I.e.

# always pair ipynb notebooks to py:percent files
formats = ['ipynb', 'py:percent']

# Pair notebooks in subfolders of 'notebooks' to scripts in subfolders of 'scripts'
[formats]
"notebooks/" = "ipynb"
"scripts/" = "py:percent"
"/path/to/mixed" = ["py:percent", "ipynb"]

Regarding the deprecation warning (or should it be a FutureWarning?): I think the earlier the better to give the users time to switch. When strictly obeying semantic versioning, the old version would need to be supported until eventually jupytext 2.0 is released.

@mwouts
Copy link
Owner Author

mwouts commented Mar 16, 2021

Thank you @JohnPaton and @grst for your feedback!

Yes Gregor I agree, we can give a try to using explicit lists... I'll let you know how it goes.

Thanks also for the other suggestions, yes sure I can use FutureWarning for this. And Jupytext 2.0 seems a bit far away for now - at least if we want to propose a text format with outputs in that version... (not planned at the moment), but anyway I see no problem with offering the backward compatibility for an extended period of time.

@mwouts mwouts force-pushed the normalize_jupytext_config_options branch from 8189a40 to 681d377 Compare March 16, 2021 23:05
@mwouts
Copy link
Owner Author

mwouts commented Mar 16, 2021

@grst I have taken most of your suggestion above regarding the formats field, which can now be either a string, a list of strings or a dict path => format - cf. the new test at f84db03

However I am not sure to see what you meant with "/path/to/mixed" = ["py:percent", "ipynb"]? Is this something that somehow works already? Or something that you'd like to see working?

@codecov
Copy link

codecov bot commented Mar 16, 2021

Codecov Report

Merging #754 (69abdd4) into master (593bcef) will decrease coverage by 0.00%.
The diff coverage is 99.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #754      +/-   ##
==========================================
- Coverage   99.10%   99.10%   -0.01%     
==========================================
  Files         106      106              
  Lines        9939    10012      +73     
==========================================
+ Hits         9850     9922      +72     
- Misses         89       90       +1     
Impacted Files Coverage Δ
jupytext/__init__.py 100.00% <ø> (ø)
jupytext/languages.py 100.00% <ø> (ø)
tests/test_metadata_filter.py 100.00% <ø> (ø)
tests/test_metadata_filters_from_config.py 100.00% <ø> (ø)
tests/test_pre_commit_scripts.py 100.00% <ø> (ø)
jupytext/config.py 97.28% <97.77%> (-0.27%) ⬇️
jupytext/cli.py 96.13% <100.00%> (-0.07%) ⬇️
jupytext/combine.py 100.00% <100.00%> (ø)
jupytext/contentsmanager.py 97.85% <100.00%> (+0.01%) ⬆️
jupytext/header.py 99.35% <100.00%> (+<0.01%) ⬆️
... and 16 more

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 e28497a...69abdd4. Read the comment docs.

@mwouts
Copy link
Owner Author

mwouts commented Mar 17, 2021

However I am not sure to see what you meant with "/path/to/mixed" = ["py:percent", "ipynb"]? Is this something that somehow works already? Or something that you'd like to see working?

FYI, if you want to use a different config in a subfolder, then you can simply create another (.)jupytext.toml file in that subfolder.

@mwouts mwouts added this to the 1.11.0 milestone Mar 17, 2021
@grst
Copy link
Contributor

grst commented Mar 17, 2021

However I am not sure to see what you meant with "/path/to/mixed" = ["py:percent", "ipynb"]? Is this something that somehow works already? Or something that you'd like to see working?

This was just to show that such constructs would be easy and legible using TOML dicts/lists. I don't think it's something used very frequently, and, as you said, putting a separate config in that folder covers that case already.

@mwouts mwouts merged commit 0a8b180 into master Mar 18, 2021
@mwouts mwouts deleted the normalize_jupytext_config_options branch March 18, 2021 05:44
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.

3 participants