Repository navigation
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.
|
|
|
||
| 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
| $ 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
priyank-p
left a comment
priyank-p
left a comment
There was a problem hiding this comment.
LGTM, with @joyeecheung's comment addressed.
|
@joyeecheung updated. i think it should be ready to land now |
b1fe9f0 to
6073324
Compare
| $ 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?
| $ 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?
6073324 to
e309178
Compare
|
@joyeecheung @gibfahn changes applied. can you please check now? |
|
I already approved, if @joyeecheung approves then it can be landed. |
4 participants
Fixes: #213