Properly parse "Fixes:"

[tniessen]

Fixes: https://github.com/joyeecheung/node-core-utils/issues/44

[apapirovski]

This seems great! Wondering two things: a) should this be case-insensitive matching and b) should we enforce the colon? "fixes #XYZ" seems valid to me...

[tniessen]

@apapirovski Good points, I think both should be changed!

[codecov-io]

Codecov Report

Merging #52 into master will increase coverage by 0.23%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
+ Coverage   96.54%   96.77%   +0.23%     
==========================================
  Files          13       13              
  Lines         434      434              
==========================================
+ Hits          419      420       +1     
+ Misses         15       14       -1

Impacted Files	Coverage Δ
lib/links.js	100% <100%> (+2.38%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5bea81c...47a6436. Read the comment docs.

[tniessen]

@apapirovski I made the checks case-insensitive. Making the colon optional requires much stricter parsing as the keywords can appear in regular sentences as well.

[addaleax]

@tniessen If I’m reading this correctly the regexes still expect the colon to always be there, right?

[apapirovski]

I feel like it's probably fine if we parse "fixes #XYZ" anywhere in the copy, no? It's possible I'm missing some downside to that approach though.

[tniessen]

If I’m reading this correctly the regexes still expect the colon to always be there, right?

@addaleax Yes, making the colon optional requires stricter filtering. I am not sure whether we can assume that any Fixes-like keyword followed by #(decimal) or by (word)/(word)/(decimal) is valid and everything else is not, but it is probably enough.

What if someone writes:

Does this fix #34234324?

This would be picked up.

[apapirovski]

We could get fancy and check the API to make sure it's an actual issue 😆 I know, I know...

It seems fine to me as is and we can revisit later, maybe including validating that these are actual open issues.

[joyeecheung]

BTW github should have some kind of API about the status of issues they consider can be automatically closed(https://help.github.com/articles/closing-issues-using-keywords/), but it doesn't seem to be very stable at this point. There is (was?) CrossReferencedEvent.willCloseTarget in the GraphQL API, we can probably revisit that later.

[joyeecheung]

@tniessen I think this can be merged now?

[tniessen]

I will revisit the version without a colon later.