Skip to content

handle id keys in cells#10

Merged
krokrob merged 3 commits intomainfrom
clean-ids
Jun 17, 2022
Merged

handle id keys in cells#10
krokrob merged 3 commits intomainfrom
clean-ids

Conversation

@gmanchon
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

gmanchon commented Jun 14, 2022

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

@brunolajoie
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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

  • notebooks
  • lab
  • VScode

works well with/without ids?

Copy link
Copy Markdown
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

Comment thread wagon_cleaner/run_clean.py Outdated
@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
Copy Markdown
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