allow git-node land to work with full PR url (#213) #219
Conversation
Codecov Report
@@ Coverage Diff @@
## master #219 +/- ##
======================================
Coverage 92.1% 92.1%
======================================
Files 18 18
Lines 659 659
======================================
Hits 607 607
Misses 52 52Continue to review full report at Codecov.
|
components/git/land.js
Outdated
| @@ -59,7 +61,7 @@ const CONTINUE = 'continue'; | |||
| const ABORT = 'abort'; | |||
|
|
|||
| function handler(argv) { | |||
| if (argv.prid && Number.isInteger(argv.prid)) { | |||
| if (argv.prid && (Number.isInteger(argv.prid) || isUrl(argv.prid))) { | |||
There was a problem hiding this comment.
We don't really need this module, do we? We can simply use a regexp to validate & capture the id: argv.prid.match(/github.com\/[^\/]+\/[^\/]+\/pull\/(\d+)/)
There was a problem hiding this comment.
@joyeecheung makes sense, also helps having less direct dependencies as they are 👍. making changes in few
components/git/epilogue.js
Outdated
| @@ -11,6 +11,7 @@ $ ncu-config set branch master # Assuming you are landing commits on master | |||
| $ git checkout master | |||
| $ git node land --abort # Abort a landing session, just in case | |||
| $ git node land $PRID # Start a new landing session | |||
| $ git node land $URL # Start a new landing session | |||
There was a problem hiding this comment.
Can you put something down in the comment indicating this is an alternative way to land a PR, not a necessary step? Something like
git node land $URL # Or, start a new landing session using the PR URL
|
@joyeecheung updated. i think it should be ready to land now |
b1fe9f0 to
6073324
Compare
| @@ -11,6 +11,7 @@ $ ncu-config set branch master # Assuming you are landing commits on master | |||
| $ git checkout master | |||
| $ git node land --abort # Abort a landing session, just in case | |||
| $ git node land $PRID # Start a new landing session | |||
| $ git node land $URL # Start a new landing session using the PR URL | |||
There was a problem hiding this comment.
Just a suggestion, but it might make more sense to merge this into the line above, and just say that $PRID can be the PR number or the PR URL.
There was a problem hiding this comment.
@gibfahn hm, i don't think it makes big difference, i think it's fine to have them on separate line? i'm okay with changing it though. just let me know...
There was a problem hiding this comment.
I also don't think it makes a big difference, which is why it was just a suggestion. If you think it's fine as is, then that's fine by me.
There was a problem hiding this comment.
cool, then ready to land i guess?
docs/git-node.md
Outdated
| @@ -32,6 +32,7 @@ $ ncu-config set branch master # Assuming you are landing commits on master | |||
| $ git checkout master | |||
| $ git node land --abort # Abort a landing session, just in case | |||
| $ git node land $PRID # Start a new landing session | |||
| $ git node land $URL # Start a new landing session | |||
There was a problem hiding this comment.
Can you update this line as well?
There was a problem hiding this comment.
@joyeecheung done, thanks for catching that
6073324 to
e309178
Compare
|
@joyeecheung @gibfahn changes applied. can you please check now? |
|
I already approved, if @joyeecheung approves then it can be landed. |
Fixes: #213