search

Jupyter-contrib / jupyter_nbextensions_configurator

A jupyter notebook serverextension providing config interfaces for nbextensions.
Other
981 stars 121 forks source link

add support for new marked version >=4.0.0 to fix #151 and support nbclassic >=0.5 #152

Closed jslorrma closed 1 year ago

jslorrma commented 1 year ago

This fixes nbclassic >=0.5 breaks jupyter_nbextensions_configurator #151

echarles commented 1 year ago

Thx @ lot @jslorrma for this fix. I have tested your branch and it works fine, the configurator is up-and-running again.

A comment for discussion, depending also on @@juhasch inputs: As this fix will work fine for nbclassic>=0.5 but will break for <5, but also for notebook, I wonder how this should be handled? We are already detecting in the nbclassic ui if the server is effectively nbclassic (the ui is also use by notebook 6.5) https://github.com/jupyter/nbclassic/blob/c0ced4f1472100f9de8e773af9b6cd6e254ce1be/nbclassic/static/notebook/js/about.js#L13

echarles commented 1 year ago

Actually this is ok as we can ask users to upgrade to the latest nbclassic or notebook 6.5.

echarles commented 1 year ago

@juhasch I don't have right to merge, could you please review? Thx a lot.

echarles commented 1 year ago

@jslorrma I thought a bit more and this would break notebook 6.4. One way to move forward is to bump the dependency in setup.py to 'notebook >=6.5 and document in the README that constraintes (the current version fo notebook 6.4, the next version for notebook 6.5 and nbclassic). Is it possible for you to update your PR with this?

jslorrma commented 1 year ago

@jslorrma I thought a bit more and this would break notebook 6.4. One way to move forward is to bump the dependency in setup.py to 'notebook >=6.5 and document in the README that constraintes (the current version fo notebook 6.4, the next version for notebook 6.5 and nbclassic).

I was thinking in the same direction yesterday night ... 👍🏻

I it possible for you to update your PR with this?

Sure. I'm a bit busy today but as this isn't a big deal I hope I can make it until this evening. Anything to adjust and take into account for the conda package build?

echarles commented 1 year ago

Thx for updates on README @juhasch

This sounds good to be merged now.

echarles commented 1 year ago

@juhasch what are your thoughts with this PR? Thx.

yuvipanda commented 1 year ago

Aha, after a bunch of digging, discovered this is what is breaking stuff for a bunch of our users :) Let us know if there is anything we can do to help move this forward.

THANK YOU FOR ALL YOUR WORK!!!

echarles commented 1 year ago

Aha, after a bunch of digging, discovered this is what is breaking stuff for a bunch of our users :) Let us know if there is anything we can do to help move this forward.

@yuvipanda Thx for raising the impact and importance of this. I think we have everything to fix that, but are missing eyes from maintainers cc @damianavila @juhasch

echarles commented 1 year ago

This has been discussed at the notebook developer meetings and followed in mail thread, see

Raising that again at today Notebook call right now.

echarles commented 1 year ago

We have just got merge rights on this repo and still need permissions to pypi for the release.

In the meantime, we should open a PR without any change on marked side that defines upper boundaries for notebook and nbclassic, merge that PR, release, and then only merge these changes.

WDYT?

yuvipanda commented 1 year ago

@echarles that seems reasonable!

Note that there are further jupyter_server related issues https://github.com/Jupyter-contrib/jupyter_nbextensions_configurator/issues/153. I tested by installing from this branch but that does not seem to fix it.

juhasch commented 1 year ago

@echarles Sorry for not responding. LGTM

Do you need anything from me, should I upload it to PyPi after merge ?

echarles commented 1 year ago

@juhasch I have finally opened https://github.com/jupyter/nbclassic/pull/236. Once merged in nbclassic and release, the changes proposed here should not be necessary. I keep you updated.

echarles commented 1 year ago

https://github.com/jupyter/nbclassic/pull/236 is merged. I will cut a new nbclassic release, so this PR is not needed anymore.

echarles commented 1 year ago

nbclassic 0.5.4 is released so closing this PR.

Do you need anything from me, should I upload it to PyPi after merge ?

@juhasch it would be good to confirm that we have enough people having acces to pypi for the releases. e.g. has @damianavila the access. Feel free to add me also.