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
Improve pattern matching of title for ignorable PRs #18
Improve pattern matching of title for ignorable PRs #18
Conversation
f5a239b
to
956bc4e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rbmrclo, the honorable first-time contributor!
Thank you for the feedback and improvement, and adding a test as well.
This change is quite reasonable to me, but before merging, would you check my review comments?
test/PullRequests.test.js
Outdated
{ title: "Don't merge - this is a PR title" }, | ||
{ title: "Do not merge - this is a PR title" }, | ||
{ title: "[DO NOT MERGE] - this is a PR title" }, | ||
{ title: "WIP - this is a PR title" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you fix the following lint error?
$ yarn lint
yarn lint v0.27.5
$ eslint .
/Users/ohbarye/.ghq/github.com/ohbarye/review-waiting-list-bot/test/PullRequests.test.js
10:42 error Missing trailing comma comma-dangle
✖ 1 problem (1 error, 0 warnings)
1 error, 0 warnings potentially fixable with the `--fix` option.
error Command failed with exit code 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! I actually didn't bother to run the lint when I was implementing this so I totally missed the comma-dangle. Fixed here b2a8aba
test/PullRequests.test.js
Outdated
{ title: "WIP - this is a PR title" } | ||
] | ||
|
||
for (let pr of pullRequests) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] const pr
is preferable here since the variable is immutable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, thanks for pointing that out! Updated here 9945d20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your quick fix! LGTM 👍
Woohoo! Thanks!!! 🚀 |
Hi @ohbarye! When we used review bot, it seems like PR titles with
[DO NOT MERGE]
or[DON'T MERGE]
are not being ignored, hence, still being returned in the list of PRs that needs review.I did a quick sanitize in order to support the ignorable words I said above. Please review!