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

Move the jupytext-config helpers to a subpackage, and add tests #1167

Merged
merged 4 commits into from
Nov 27, 2023

Conversation

mwouts
Copy link
Owner

@mwouts mwouts commented Nov 23, 2023

Closes #1097

@mwouts
Copy link
Owner Author

mwouts commented Nov 23, 2023

@parmentelat does this look fine to you? Thanks!

@mwouts mwouts mentioned this pull request Nov 23, 2023
Copy link
Contributor

@parmentelat parmentelat left a comment

Choose a reason for hiding this comment

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

looks motly good to me except for the way we can configure the actual config file

self.logger = logger or logging.getLogger(__name__)
self.config = {}
# the state before any changes
self._prior_config = {}
self.settings_file = Path(settings_path) / "default_setting_overrides.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

imho if we're to be able to provide the location for the settings, i'td be better if the caller provides the whole path to the config file including its basename
my reason for that is, the configuration scheme within jupyterlab is truly complex, and with for example the overrides.d scheme, we may someday need to use another filename than this hard-coded default_setting_overrides.json

which amounts to dealing only with a single settings_path all over the place

@mwouts mwouts added this to the 1.16.0 milestone Nov 25, 2023
Copy link

codecov bot commented Nov 25, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c37d9c2) 97.59% compared to head (15452de) 97.61%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1167      +/-   ##
==========================================
+ Coverage   97.59%   97.61%   +0.02%     
==========================================
  Files          26       29       +3     
  Lines        4357     4451      +94     
==========================================
+ Hits         4252     4345      +93     
- Misses        105      106       +1     
Flag Coverage Δ
external 75.23% <ø> (+0.11%) ⬆️
functional 88.54% <ø> (ø)
integration 77.37% <96.22%> (+0.53%) ⬆️
unit 66.64% <91.11%> (+0.25%) ⬆️

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 merged commit 405617f into main Nov 27, 2023
29 checks passed
@mwouts mwouts deleted the coverage_jupytext_config branch November 27, 2023 20:48
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.

Restore the coverage
3 participants