nodejs / node-core-utils

CLI tools for Node.js Core collaborators
https://nodejs.github.io/node-core-utils/
MIT License
256 stars 113 forks source link

Detect merged/closed PR #137

Closed gibfahn closed 6 years ago

gibfahn commented 6 years ago

Currently if you run get-metadata on a closed PR, you don't get an obvious indication that it's closed.

▶▶▶ get-metadata 17899                                                                                                                                                                                                                ~/wrk/com/node-core-utils (xdg-config✦)
✔  Done loading data for nodejs/node/pull/17899
----------------------------------- PR info ------------------------------------
Title   doc: remove x86 from os.arch() options #17899
Author  gibfahn
Commits 1
Branch  gibfahn:process-arch -> nodejs:master
Labels  doc
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/17899
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
--------------------------------------------------------------------------------
✔  Requested Changes: 0
✔  Approvals: 7, 3 from TSC (jasnell, cjihrig, danbev)
⚠  Commits were pushed since the last review:
⚠  - doc: remove x86 from os.arch() options
ℹ  Last Full CI on 2018-01-03T22:59:42Z: https://ci.nodejs.org/job/node-test-commit/15215/
⚠  Commits were pushed after the last Full CI run:
⚠  - doc: remove x86 from os.arch() options

It should probably be made clear that the PR is closed.

tniessen commented 6 years ago

Would a warning suffice or should this be an error?

gibfahn commented 6 years ago

Would a warning suffice or should this be an error?

I'm not sure, I was thinking that a non-zero exit code would be useful if you were scripting this, but I haven't actually scripted it so I'm not sure...

I was also thinking that maybe it shouldn't generate metadata, but again, there may be use-cases where you actually want to get metadata for closed PRs, so IDK about that either.

Basically IDK, that's why I opened this as an issue, to see what people think 😁 .

Maybe just a big 🔴 - PR is closed (Merged) would be adequate.

joyeecheung commented 6 years ago

It's hard to tell if it's actually merged given the way we land things, but a warning about the PR being closed is definitely doable.

joyeecheung commented 6 years ago

...oh right, we can potentially do a git log and grep for PR-URLs.

gibfahn commented 6 years ago

We can at least tell if it's merged if it's a collaborator's PR (e.g. https://github.com/nodejs/node/pull/17899).

Alternatively maybe look for a landed in comment.

priyank-p commented 6 years ago

We can check and tell if PR is closed: landed by collaborator or merged: landed by the collaborator who open a PR.

I was thinking a warning will be suffice since the metadata would be generated and non-zero.