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

Properly parse "Fixes:" #52

Merged
merged 2 commits into from Nov 3, 2017

Conversation

Projects
None yet
6 participants
@tniessen
Member

tniessen commented Nov 2, 2017

@@ -6,18 +6,21 @@ const assert = require('assert');
const htmls = fixtures.readJSON('op_html.json');
const expected = [{
fixes: ['https://github.com/node/issues/16437'],
fixes: ['https://github.com/nodejs/node/issues/16437'],

This comment has been minimized.

@tniessen

tniessen Nov 2, 2017

Member

I believe this test was incorrect before. Not sure whether I am supposed to pass nodejs/node as the repo name.

@tniessen

tniessen Nov 2, 2017

Member

I believe this test was incorrect before. Not sure whether I am supposed to pass nodejs/node as the repo name.

This comment has been minimized.

@joyeecheung

joyeecheung Nov 2, 2017

Member

I think we can just pass the repo + the owner name to LinkParser

@joyeecheung

joyeecheung Nov 2, 2017

Member

I think we can just pass the repo + the owner name to LinkParser

This comment has been minimized.

@joyeecheung

joyeecheung Nov 2, 2017

Member

^to be clear, I meant changing the signature of the constructor of LinkParser to (owner, repo, html)

@joyeecheung

joyeecheung Nov 2, 2017

Member

^to be clear, I meant changing the signature of the constructor of LinkParser to (owner, repo, html)

@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Nov 2, 2017

Member

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...

Member

apapirovski commented Nov 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Nov 2, 2017

Member

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

Member

tniessen commented Nov 2, 2017

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

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Nov 2, 2017

Codecov Report

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

Impacted file tree graph

@@            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%) ⬆️

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.

codecov-io commented Nov 2, 2017

Codecov Report

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

Impacted file tree graph

@@            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%) ⬆️

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

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Nov 2, 2017

Member

@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.

Member

tniessen commented Nov 2, 2017

@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.

@Tiriel

Tiriel approved these changes Nov 2, 2017

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 2, 2017

Member

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

Member

addaleax commented Nov 2, 2017

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

@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Nov 2, 2017

Member

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.

Member

apapirovski commented Nov 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Nov 2, 2017

Member

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.

Member

tniessen commented Nov 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Nov 3, 2017

Member

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.

Member

apapirovski commented Nov 3, 2017

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

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Nov 3, 2017

Member

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.

Member

joyeecheung commented Nov 3, 2017

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

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Nov 3, 2017

Member

@tniessen I think this can be merged now?

Member

joyeecheung commented Nov 3, 2017

@tniessen I think this can be merged now?

@tniessen tniessen merged commit 677bf31 into nodejs:master Nov 3, 2017

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk No new issues
Details
@tniessen

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Nov 3, 2017

Member

I will revisit the version without a colon later.

Member

tniessen commented Nov 3, 2017

I will revisit the version without a colon later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment