Reviewing the content here

[TomAugspurger]

@stijnvanhoey @jorisvandenbossche I'd like to go through the notebooks and leave comments on the content. Can we install https://github.com/marketplace/review-notebook-app/plan/MDIyOk1hcmtldHBsYWNlTGlzdGluZ1BsYW4yMDIz#pricing-and-setup on this repo and make a dummy PR that just moves all the notebooks to a different folder, so that there's a diff to comment on? Or maybe a bunch of PRs, 1 per notebook?

[jorisvandenbossche]

Ah, that seems like a good idea! I suppose @stijnvanhoey will need to install the app in this repo, I don't have admin access

[stijnvanhoey]

Thanks! the app is activated

[jorisvandenbossche]

OK, shall we create dummy PRs for each notebook or a single big PR? I don't know how much the discussion might get intertwined between notebooks, but otherwise one per notebook is maybe better.

[TomAugspurger]

I think one per notebook is best. I was just going to make one by renaming a file, but GitHub (correctly) is viewing it as a moved file. Not sure how to trick it into thinking otherwise (maybe a commit deleting, and a commit restoring).

…

On Tue, Sep 10, 2019 at 2:17 PM Joris Van den Bossche < ***@***.***> wrote: OK, shall we create dummy PRs for each notebook or a single big PR? I don't know how much the discussion might get intertwined between notebooks, but otherwise one per notebook is maybe better. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1?email_source=notifications&email_token=AAKAOIX3BXDDHO6MI5JTV2DQI7XFBA5CNFSM4IVGRUDKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6MGLEQ#issuecomment-530081170>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKAOIQGJK4UJPQFLT5C6BDQI7XFBANCNFSM4IVGRUDA> .

[jorisvandenbossche]

We can maybe make a new empty "review" branch and point the PRs to merge in there?

[jorisvandenbossche]

I created a test PR: #2

Any idea on how now to use the review app?

[jorisvandenbossche]

Ah, there is comment of the app ;-)

[jorisvandenbossche]

Created PRs for all notebooks

[justmarkham]

Currently, there are no PRs for notebooks 9 or 10. Please let me know once those exist, and I'd be happy to leave my comments on those notebooks (via ReviewNB). Thanks!

P.S. I suspect I could create those PRs myself, but I'm not sure how, and I didn't want to screw anything up with ReviewNB! Thanks for your assistance 👍

[jorisvandenbossche]

I created PRs for those two notebooks as well.

Note to future self and others (so I don't need to google it yet another time ;-)), how I made the branches:

# "review" is the branch with all notebooks deleted
git checkout review
# create a new branch starting from this review branch
git checkout -b review-notebook-10-text
# add a single file from the master branch
git checkout master notebooks/10_text_data.ipynb
git commit -am "Add 10_text_data.ipynb from master"
# and push this, creating a PR against the review branch

and now, to update a PR, you can do something similar, assuming your master has the latest changes:

git checkout review-notebook-10-text
git checkout master notebooks/10_text_data.ipynb
git commit -am "update from master"