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

handle id keys in cells #10

Merged
merged 3 commits into from
Jun 17, 2022
Merged

handle id keys in cells #10

merged 3 commits into from
Jun 17, 2022

Conversation

gmanchon
Copy link
Contributor

@gmanchon gmanchon commented Jun 14, 2022

I had not worked on a notebook for a long time and it felt awful to deal with the ids

We have discussed this before and I am not aware that a use case for these ever changing keys has appeared ?

So it would feel safe and quite practical to remove them, WDYT ?

BEFORE

me : let's look at the diff now before I commit 👀
me : oh, that's underwhelming 🙁

Screenshot 2022-06-14 at 11 26 52

me : scrolling... scrolling... scrolling...
me : oh, there you are, now I can confidently scroll more to make sure everything is ok ☹️

Screenshot 2022-06-14 at 11 27 58

AFTER

me : me feeling like 👨‍🎨

Screenshot 2022-06-14 at 11 30 08

@gmanchon gmanchon requested a review from krokrob June 14, 2022 09:37
@gmanchon gmanchon mentioned this pull request Jun 14, 2022
@gmanchon
Copy link
Contributor Author

gmanchon commented Jun 14, 2022

@krokrob looks like this repo does not have access to the Gemfury credentials

@brunolajoie
Copy link

brunolajoie commented Jun 15, 2022

@gmanchon what are the id for ? Is it a new thing? Dyou think they are used by any means somewhere?
Have a look at this or this maybe it brings some answers?

@gmanchon
Copy link
Contributor Author

@brunolajoie thanks for the links. The content seems to suggest that the goal of the id field is to allow the kernel to refer to the cells either within a notebook session (during an execution) or accros sessions.

Since the ids vary every time I save my notebook, I struggle to understand how they would allow an app to reference a cell reliably accros notebook sessions.

On the contrary, if the goal is to allow an app to reference a cell within a notebook session, I struggle to understand how persisting the data on disk helps...

My best guess at the moment is that the specification is ongoing or not yet supported by all the tools of the ecosystem.

Anyways I suppose we can get rid of these changing values until we identify a pain point (if any ever emerges), in which case we will be able to quickly revert to the previous nbcleanmeta package behavior. WDYT ?

@brunolajoie
Copy link

Let's do that, but before merging, can you just check that

  • notebooks
  • lab
  • VScode

works well with/without ids?

Copy link
Member

@krokrob krokrob left a comment

Choose a reason for hiding this comment

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

LGTM @gmanchon I just have 1 question before merging

@krokrob krokrob merged commit 090cba9 into main Jun 17, 2022
@krokrob krokrob deleted the clean-ids branch June 17, 2022 09:24
@gmanchon
Copy link
Contributor Author

gmanchon commented Jun 17, 2022

@krokrob @brunolajoie as a wrap-up, jupyter notebook, lab and vscode seem to work pretty well without the ids

the ids appear with nbformat version 4.5

at the moment I was not able to determine precisely what actions cause the ids to reset other than it frequently is the case when working on the lectures

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