-
Notifications
You must be signed in to change notification settings - Fork 111
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
allow passing full URLs on the command line #6
Conversation
bin/metadata.js
Outdated
// Fast path: numeric string | ||
if (`${+id}` === id) | ||
return +id; | ||
const match = id.match(/^https:.*\/([0-9]+)$/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps try new URL(id)
, and fallback if it errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, question: fall back to what? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh... good point ;-) ... try new url.URL(id)
, check the protocol, and error if it's not a valid https URL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a \/?
at the end because sometimes people do paste the slash, like https://github.com/nodejs/node/pull/14782/
(which breaks branch-diff
from time to time..)...also maybe we can just make the regexp out of repo
and owner
? That way we can validate it's a pull instead of an issue as well (right now if it's an issue we just throw the errors around).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah wait, we don't provide command line options for repo
/owner
right now :/ OK maybe just validate pull/([0-9])+\/?
part..I know there are people in other repos using node-review
so that should be a todo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup – I’ve added checks for pull/
and /
or /files
at the end :)
bin/metadata.js
Outdated
@@ -25,7 +25,7 @@ const MetadataGenerator = require('../lib/metadata_gen'); | |||
const OWNER = 'nodejs'; | |||
const REPO = 'node'; | |||
|
|||
const PR_ID = parseInt(process.argv[2]); // example | |||
const PR_ID = parsePRId(process.argv[2]); // example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the // example
comment can be removed now XD I added it when I chose 14782 as the default PR to test...I can do that later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done! :)
bin/metadata.js
Outdated
// Fast path: numeric string | ||
if (`${+id}` === id) | ||
return +id; | ||
const match = id.match(/^https:.*\/([0-9]+)$/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a \/?
at the end because sometimes people do paste the slash, like https://github.com/nodejs/node/pull/14782/
(which breaks branch-diff
from time to time..)...also maybe we can just make the regexp out of repo
and owner
? That way we can validate it's a pull instead of an issue as well (right now if it's an issue we just throw the errors around).
i.e. make `get-metadata nodejs/node#16489 work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (I subconsciously opened ci.nodejs.org and...wait, we don't have test and we don't have CI yet, lol)
I will just use the squash and merge button here...thanks! |
i.e. make
get-metadata https://github.com/nodejs/node/pull/16489
work