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

Delay list -> dict conversation to end #192

Merged
merged 3 commits into from
Dec 22, 2021
Merged

Delay list -> dict conversation to end #192

merged 3 commits into from
Dec 22, 2021

Conversation

vidartf
Copy link
Member

@vidartf vidartf commented Jul 21, 2021

This should prevent dicts in webapp data from being clobbered by converted lists from the settings_dir.

Fixes #191 .

  • TODO: Add tests.

@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2021

Codecov Report

Merging #192 (cdc21cd) into main (04fcf0e) will increase coverage by 0.45%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #192      +/-   ##
==========================================
+ Coverage   77.48%   77.93%   +0.45%     
==========================================
  Files          28       28              
  Lines        2105     2121      +16     
==========================================
+ Hits         1631     1653      +22     
+ Misses        474      468       -6     
Impacted Files Coverage Δ
jupyterlab_server/config.py 66.27% <100.00%> (+2.90%) ⬆️
jupyterlab_server/handlers.py 88.09% <100.00%> (+0.29%) ⬆️
jupyterlab_server/pytest_plugin.py 93.65% <100.00%> (+0.31%) ⬆️
jupyterlab_server/tests/test_labapp.py 100.00% <100.00%> (ø)

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 04fcf0e...cdc21cd. Read the comment docs.

@blink1073 blink1073 added the bug label Jul 21, 2021
@blink1073 blink1073 added this to the 2.6 milestone Jul 21, 2021
@blink1073 blink1073 modified the milestones: 2.6, 2.7 Aug 11, 2021
@blink1073 blink1073 modified the milestones: 2.7, 2.6, 2.8 Sep 7, 2021
This should prevent dicts in webapp data from being clobbered by converted lists from the settings_dir.
@vidartf vidartf marked this pull request as ready for review December 21, 2021 19:22
@vidartf
Copy link
Member Author

vidartf commented Dec 21, 2021

@minrk This setting will interact with the hook introduced in #220 by expecting certain keys to now map to dicts instead of lists. Have you been using any such keys in hub yet? (see the added test for details)

@minrk
Copy link
Contributor

minrk commented Dec 22, 2021

I haven't yet, but it shouldn't be a problem for us. Thanks for the heads up.

@jtpio
Copy link
Member

jtpio commented Dec 27, 2021

This setting will interact with the hook introduced in #220 by expecting certain keys to now map to dicts instead of lists.

Should this be considered a breaking change?

After investigating the JupyterLab CI failure in jupyterlab/jupyterlab#11744, it looks like consumers retrieving the page config with get_page_config now get dicts instead of lists. This is causing issues with disabled extensions in JupyterLab with jupyterlab_server>=2.10 when using the CLI (jupyterlab/jupyterlab#11744 (comment)).

This could be fixed in lab, but would need to be able to handle both dicts and lists to allow for various combinations of jupyterlab and jupyterlab_server (< 2.10 or >=2.10).

blink1073 added a commit to blink1073/jupyterlab_server that referenced this pull request Dec 28, 2021
blink1073 added a commit that referenced this pull request Dec 28, 2021
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 this pull request may close these issues.

Attempted config merge of list and dict clobbers dict
5 participants