Skip to content
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

get-metadata: should we accept "Fixes" and "Refs" without ":"? #127

Closed
vsemozhetbyt opened this issue Nov 25, 2017 · 9 comments
Closed

get-metadata: should we accept "Fixes" and "Refs" without ":"? #127

vsemozhetbyt opened this issue Nov 25, 2017 · 9 comments
Labels
enhancement Things that enhances functionality, provided by node-core-utils

Comments

@vsemozhetbyt
Copy link
Contributor

Refs: in the nodejs/node#17295, the Fixes https://github.com/nodejs/node/issues/17294 commit message line was not included in the metadata.

@joyeecheung
Copy link
Member

joyeecheung commented Nov 25, 2017

I am a bit afraid this would give many false positives and we have to end up with struggling with natural language processing (I don't have problem with that but it's a really big undertake if you want to get it right). So the easiest solution is: if people don't follow the format, just ignore it.

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Nov 25, 2017

@joyeecheung I am a bit concerned that people may use GitHub syntax that does not require :

https://help.github.com/articles/closing-issues-using-keywords/#closing-an-issue-in-the-same-repository

@Tiriel
Copy link
Contributor

Tiriel commented Nov 25, 2017

Well, they might indeed, but the syntax only works when the branch is merged according to the doc you linked, I don't know if it works with our process.

Anyway, I'm +1 with @joyeecheung , the formatting exists for a reason IMHO, I don't think we should make allowance or we risk having more problems than now.

@targos
Copy link
Member

targos commented Nov 25, 2017

is it detected by a regexp now?

@joyeecheung
Copy link
Member

@targos Yea it's deteced by regular expressions in lib/links.js, with some JSdom queries to get the URL out of the href attributes.

@targos
Copy link
Member

targos commented Nov 28, 2017

What about something like this?

/(Close[ds]?|Fix(e[ds])?|Resolve[sd]?)\s*:?\s*https?:\/\/(\S+)/i;

I think we require full URLs already so this should be fine and without false positives.

That said, I'm not sure we should encourage this syntax. Maybe it could be normalized by get-metadata.

@gibfahn
Copy link
Member

gibfahn commented Jan 3, 2018

That said, I'm not sure we should encourage this syntax. Maybe it could be normalized by get-metadata.

Agreed, having get-metadata be forgiving in what it accepts is good, but it should only output the one canonical form.

@joyeecheung joyeecheung added the enhancement Things that enhances functionality, provided by node-core-utils label Jan 18, 2018
@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot added the stale label Aug 18, 2020
@codebytere
Copy link
Member

codebytere commented Aug 18, 2020

i'd say we should close this - I see no real reason to deviate from syntax we've been adhering to for years. Feel free to open again if yall have other thoughts!

@targos targos removed the stale label Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Things that enhances functionality, provided by node-core-utils
Projects
None yet
Development

No branches or pull requests

6 participants