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

Revert "Delay list -> dict conversation to end (#192)" #234

Merged
merged 1 commit into from
Dec 28, 2021

Conversation

blink1073
Copy link
Contributor

@blink1073 blink1073 commented Dec 28, 2021

This reverts commit 7b9c024.

Temporary fix for jupyterlab/jupyterlab#11743

@codecov-commenter
Copy link

codecov-commenter commented Dec 28, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@cce8b4e). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #234   +/-   ##
=======================================
  Coverage        ?   76.08%           
=======================================
  Files           ?       29           
  Lines           ?     2204           
  Branches        ?        0           
=======================================
  Hits            ?     1677           
  Misses          ?      527           
  Partials        ?        0           

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 cce8b4e...3bcbb93. Read the comment docs.

@blink1073 blink1073 merged commit cbe3b92 into jupyterlab:main Dec 28, 2021
@blink1073 blink1073 deleted the revert-192 branch December 28, 2021 22:41
@fcollonval
Copy link
Member

I think we can reapply #192 now that jupyterlab/jupyterlab#11744 is merged?
cc @jtpio

@jtpio
Copy link
Member

jtpio commented Dec 30, 2021

It's not clear whether #192 should be considered as a bug fix or not. If so and assuming get_page_config is not really part of the public API then we could reapply it as a bug fix, now that jupyterlab/jupyterlab#11744 handles both cases.

@fcollonval
Copy link
Member

Ok so it would be good to wait for a feedback of @vidartf before taking a decision.

I added a note to bring that up at the next meeting.

@vidartf
Copy link
Member

vidartf commented Jan 5, 2022

Sorry, I was out for the holidays. Did that change break anything more than the test? get_page_config only gives the config from disk, and not the "extra" settings that the handler adds, so it has a somewhat limited scope already. The issue is ofc that it modifies the type of some returned values. Since the dict -> list conversion is mainly intended to be for the benefit of the frontend, I would argue that it makes sense to only apply this transform right before it sends the data.

#192 can be considered more of an enhancement, but it can be argued as a bug fix in that the values in page_config_data are not applied as you would expect.

@jtpio
Copy link
Member

jtpio commented Jan 5, 2022

Did that change break anything more than the test?

Looks like it affected the disabling and enabling of JupyterLab extensions, based on this comment: jupyterlab/jupyterlab#11744 (comment)

#192 can be considered more of an enhancement, but it can be argued as a bug fix in that the values in page_config_data are not applied as you would expect.

So we could maybe reapply #192? Especially now that jupyterlab/jupyterlab#11744 should handle both cases?

@vidartf
Copy link
Member

vidartf commented Jan 6, 2022

So we could maybe reapply #192?

Probably, but it might not make sense as a patch/minor release if it breaks older versions of lab.

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.

5 participants