-
Notifications
You must be signed in to change notification settings - Fork 572
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
Changed clearmetadata preprocessor to remove all metadata by default #1314
Conversation
Gonna merge this in another day or two if no one any comments fyi. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good to me. And it makes sense. I think this will be a nice change for people that specifically want to not have things like "version":3.6.5"
committed to git. It also makes sense to get rid of the kernelspec (since 2 different machines will run the notebook with different kernels). But I have a couple discussion points:
- Should we maybe include the
"language_info"
in the default metadata keys to preserve? So that we don't need to guess the language of the notebook: Add--clean
option to--to notebook
to strip all metadata and output? #637 (comment)
A user could always override it later, and it doesn't look like anything in that field should change frequently.
"metadata": {
"kernelspec": {
"display_name": "Python 3",
"language": "python",
"name": "python3"
},
"language_info": {
"codemirror_mode": {
"name": "ipython",
"version": 3
},
"file_extension": ".py",
"mimetype": "text/x-python",
"name": "python",
"nbconvert_exporter": "python",
"pygments_lexer": "ipython3",
"version": "3.6.5"
}
}
- Should we give users the option to clear notebook metadata but not cell metadata?
Thanks for the review. I think I can incorporate those suggested changes fairly easily |
I just realised that |
a3fccd8
to
9977c33
Compare
@SylvainCorlay or @t-makaro If you wanted to quickly review this for the release tomorrow I believe it's addressed the PR comments. |
I am not too opinionated about this. Leaving this to you judgement @t-makaro @jupyter/everybody |
I'm going to merge for now since I think the comments are well addressed and tested to avoid blocking release stuff |
If we need to adjust later I can make tweaks |
I decided to just go ahead and implement the requested change for #637. Technically it changes the default behavior to clear notebook and cell metadata, but I think that's what's usually intended on first-use and I added a flag to fall back to the old behavior if need-be. I also added a flag for preserving specific keys by name.