Uh oh!
There was an error while loading. Please reload this page.
add support for checking new issue against issue templates - #39
Conversation
Thanks so much for opening your first PR here! |
shiftkey
commented
Sep 25, 2018
I think this is ready for a review. I mostly just reused the existing test infrastructure, which was very helpful to ensure I hadn't broken anything! |
hiimbex
left a comment
There was a problem hiding this comment.
Thanks for taking this on @shiftkey! Super excited to see some progress here. 🎉
I left some thoughts/feedback in the comments, but the tldr is that this appears to suffer from the same issues that have prevented me from implementing this feature, which is how to actually pattern match against an issue template.
| badBody = true | ||
| } | ||
| } else if (eventSrc === 'issue') { | ||
| if (!(await IssueBodyChecker.isBodyValid(body, config, context))) { |
There was a problem hiding this comment.
What do you think about check config.checkIssueTemplate here?
There was a problem hiding this comment.
This was me following the pattern that PullRequestBodyChecker used. I can move it out, just wanting to confirm I should ignore that.
There was a problem hiding this comment.
Hah! Typical that my code would contradict me. I didn't actually write PullRequestBodyChecker, but yea no need to match that pattern exactly. I think I'd rather not even call this function if that config option isn't listed
There was a problem hiding this comment.
This is very minor, so I'll defer to you on whether matching the existing code or being slightly more efficient is better. Let me know which you decide @shiftkey and happy to merge this whenever.
There was a problem hiding this comment.
How about we leave this as-is, and I'll follow up with a PR to lift both this config.checkIssueTemplate and config.checkPullRequestTemplate out of their relevant components and into here, so they remain consistent?
| return false | ||
| } | ||
| if (config.checkIssueTemplate && (await isBodyEqualToTemplate(body, context))) { |
There was a problem hiding this comment.
I'd recommend moving this check config.checkIssueTemplate in accordance with my previous comment!
| try { | ||
| const singleTemplate = await context.github.repos.getContent({ | ||
| owner: context.payload.repository.owner.login, | ||
| repo: context.payload.repository.name, |
There was a problem hiding this comment.
Can we clean this up using some probot internal APIs:
constsingleTemplate=awaitcontext.github.repos.getContent(context.repo({path}))| try { | ||
| const templatesDirectory = await context.github.repos.getContent({ | ||
| owner: context.payload.repository.owner.login, | ||
| repo: context.payload.repository.name, |
Uh oh!
There was an error while loading. Please reload this page.
Congrats on merging your first pull request here! 🎉 How awesome! |
hiimbex
commented
Oct 18, 2018
Sorry for the delay! Thanks again for this @shiftkey 🎉 |
Fixes#8