search

nodejs / node-core-utils

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

get-metadata: missing "Refs:" in metadata section #156

Closed vsemozhetbyt closed 3 years ago

vsemozhetbyt commented 6 years ago

For https://github.com/nodejs/node/pull/18310, "Refs:" was omitted in metadata section.

priyank-p commented 6 years ago

@vsemozhetbyt can provide more details on this or do you have any clue why that happend?

vsemozhetbyt commented 6 years ago

@cPhost I cannot understand the cause. Commit message:

doc: remove confusing signature in fs.md

Fixes: https://github.com/nodejs/node/issues/18305
Refs: https://github.com/nodejs/node/pull/13424
get-metadata output: ```console $ winpty get-metadata.cmd 18310 √ Done loading data for nodejs/node/pull/18310 ----------------------------------- PR info ------------------------------------ Title doc: remove confusing signature in fs.md (#18310) Author Vse Mozhet Byt (@vsemozhetbyt) Branch vsemozhetbyt:doc-fs-del-err-signature -> nodejs:master Labels doc, fast-track, fs Commits 1 - doc: remove confusing signature in fs.md Committers 1 - Vse Mozhet Byt ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/18310 Fixes: https://github.com/nodejs/node/issues/18305 Reviewed-By: Joyee Cheung Reviewed-By: Gireesh Punathil Reviewed-By: Richard Lau -------------------------------------------------------------------------------- √ Requested Changes: 0 √ Approvals: 3, 1 from TSC (joyeecheung) i Last Full CI on 2018-01-23T03:47:53Z: https://ci.nodejs.org/job/node-test-pull-request-lite/115/ i This PR is being fast-tracked ‼ This PR is closed ```

Seems to be a common case, [`REFS_RE`](https://github.com/nodejs/node-core-utils/blob/8e3d831dc5deee9b6e053f5a9f26b30b410cfaa0/lib/links.js#L5) is matched.
priyank-p commented 6 years ago

Aren't you suppose to add Refs: <url> in for get-metadata it to detect it?

gibfahn commented 6 years ago

Aren't you suppose to add Refs: in for get-metadata it to detect it?

You can add it in the commit message or the first comment in the issue. Looks like @vsemozhetbyt added it to the commit message.

priyank-p commented 6 years ago

Ah yeah, I don't know what i was thinking...

priyank-p commented 6 years ago

@joyeecheung it looks this is failing because github is once again using .issue-link or something and the url is not being parsed.

I don't know why this did not came to my mind earlier but we can avoid using jsdom simply by using bodyText field rather than bodyHTML field. And avoid other bugs caused by it.

joyeecheung commented 6 years ago

@cPhost I think the OP is about the URL in the commit message not being parsed, rather than the body of the PR?

priyank-p commented 6 years ago

Yep, found that it, we need to also pass commit msg into linkParser and then get refs from one of them. I think

joyeecheung commented 6 years ago

@cPhost In that case it's not a bug caused by .issue-link, the jsdom parsing is there to work around text like Refs: #XXXXX. We should use something else for commit messages instead.

priyank-p commented 6 years ago

@joyeecheung so we can just combine bodyHTML and messageBodyHTML and just passed to LinkParse should fix it.

github-actions[bot] commented 3 years ago

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

priyank-p commented 3 years ago

I think this is no longer an issue since it didn't come up recently. And, my comments are all but un-understandable back then :(