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

Sl/pre commit hook #21

Closed
wants to merge 4 commits into from
Closed

Sl/pre commit hook #21

wants to merge 4 commits into from

Conversation

sglyon
Copy link
Member

@sglyon sglyon commented Oct 22, 2016

This adds a python script I use as a pre-commit to remove all output cells from any notebook in the repo whenever I try to do git commit.

I'm not 100% sure I like this.

On the plus side, it eliminates an annoyance I've had where I open up a notebook to lecture from and all the output is already there. This is annoying because

  1. it makes the notebook harder to navigate
  2. It doesn't come with any "spoiler alert!" warning -- students can just see exactly what I'm about to tell them
  3. Makes the files much larger than they need to be

On the negative side, we can no longer get a preview of any images or results from github or in nbviewer.

I'm up in the air about this.

Let's have a vote amongst all current and (known) future instructors -- majority wins.

For the record, my vote is in favor of this change.

cc @cc7768 @mwaugh0328 @szokeb87 @danielcsaba

@cc7768
Copy link
Member

cc7768 commented Oct 22, 2016

I vote in favor. Nice to not have the punchline spoiled sometimes.

@sglyon sglyon mentioned this pull request Oct 22, 2016
@szokeb87
Copy link
Contributor

I vote in favor. I find the advantages convincing

@danielcsaba
Copy link
Member

Hey guys,
I would also vote in favor.

@sglyon
Copy link
Member Author

sglyon commented Oct 26, 2016

Done in 92eba49

@sglyon sglyon closed this Oct 26, 2016
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.

None yet

4 participants