Skip to content

Add the Notification API #147

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

Closed
wants to merge 2 commits into from
Closed

Conversation

coreyauger
Copy link
Contributor

No description provided.

@mseddon
Copy link
Contributor

mseddon commented Sep 1, 2015

Oh, you don't need to create a new PR to change this- you can simply "git push origin [branch] -f" and github is smart enough to update the code but keep the notes. I did this first time I committed here #14 :)

To quote Sébastien when I did it: "this is better than reopening a new PR because comments are kept and correlated with the new commits for lines that have not changed."

@coreyauger
Copy link
Contributor Author

doh! .. sorry I did a re-checkout. I also forgot to update the import in the example which i have done now. How do I reissue the pull request with the fix?

@mseddon
Copy link
Contributor

mseddon commented Sep 1, 2015

👍 got it before I finished typing. The only other issue is you're PR'ing from master (should have caught this earlier, blah) onto master which causes issues if we need to go through history. The PR should be a single commit for this, in a branch, say notification-api

@mseddon
Copy link
Contributor

mseddon commented Sep 1, 2015

The way to do this is first wind back your working directory two commits, but keep your changes. git reset --soft HEAD~2 should put you there. Once that is done you can git checkout -b notification-api, and commit these fixes as usual (git commit -m "Add the Notification API"), then git push origin notification-api. One moment while I find out if there is a way to modify the branch of an in-flight PR.

@mseddon
Copy link
Contributor

mseddon commented Sep 1, 2015

bleh. isaacs/github#18 seems it's not possible. once you have a notification-api branch on your fork with your changes, you will have to send another PR from your fork's notification-api branch onto our master and close this one. :)

@coreyauger
Copy link
Contributor Author

Ok thanks for your help 👍

@coreyauger coreyauger closed this Sep 1, 2015
@mseddon
Copy link
Contributor

mseddon commented Sep 1, 2015

np. my first few PR's were a total car accident ;)

@mseddon mseddon mentioned this pull request Sep 1, 2015
@coreyauger
Copy link
Contributor Author

I have used this line twice already.. lol :)

@mseddon
Copy link
Contributor

mseddon commented Sep 3, 2015

;) thanks for the PR 👍

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.

2 participants