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

Changed clearmetadata preprocessor to remove all metadata by default #1314

Merged
merged 2 commits into from
Sep 8, 2020

Conversation

MSeal
Copy link
Contributor

@MSeal MSeal commented Jul 2, 2020

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.

@MSeal MSeal requested review from SylvainCorlay and t-makaro July 2, 2020 05:41
@MSeal
Copy link
Contributor Author

MSeal commented Jul 20, 2020

Gonna merge this in another day or two if no one any comments fyi.

Copy link
Contributor

@t-makaro t-makaro left a 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:

  1. 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"
  }
 }
  1. Should we give users the option to clear notebook metadata but not cell metadata?

nbconvert/preprocessors/clearmetadata.py Show resolved Hide resolved
@MSeal
Copy link
Contributor Author

MSeal commented Jul 20, 2020

Thanks for the review. I think I can incorporate those suggested changes fairly easily

@t-makaro
Copy link
Contributor

t-makaro commented Jul 20, 2020

I just realised that "language_info" is more nested than I thought. It's specifically, the name subfield of language_info that I want to keep which I guess makes it a bit harder. The rest of it can be tossed.

@MSeal MSeal mentioned this pull request Aug 24, 2020
4 tasks
@MSeal MSeal force-pushed the clearAllTheMetadata branch from a3fccd8 to 9977c33 Compare September 7, 2020 20:02
@MSeal
Copy link
Contributor Author

MSeal commented Sep 7, 2020

@SylvainCorlay or @t-makaro If you wanted to quickly review this for the release tomorrow I believe it's addressed the PR comments.

@SylvainCorlay
Copy link
Member

I am not too opinionated about this.

Leaving this to you judgement @t-makaro @jupyter/everybody
@maartenbreddels let me know if you want to chime in on this before the release!

@MSeal
Copy link
Contributor Author

MSeal commented Sep 8, 2020

I'm going to merge for now since I think the comments are well addressed and tested to avoid blocking release stuff

@MSeal
Copy link
Contributor Author

MSeal commented Sep 8, 2020

If we need to adjust later I can make tweaks

@MSeal MSeal merged commit db0e589 into jupyter:master Sep 8, 2020
@MSeal MSeal added this to the 6.0 milestone Sep 8, 2020
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