Repository navigation
Detecting w i p commits - #32
Conversation
| number: context.payload.pull_request.number | ||
| }) | ||
|
|
||
| if (commits.data.some(element => { return /\b(wip)\b/i.test(element.commit.message) })) { |
There was a problem hiding this comment.
I think it makes sense to re-use isWip from above, if you extract it as a function and pass string argument to it
There was a problem hiding this comment.
Thanks for the review @HeeL ! I pushed a commit that follows your suggestion
| async function handlePullRequestChange (robot, context) { | ||
| const title = context.payload.pull_request.title | ||
| const isWip = /\b(wip|do not merge)\b/i.test(title) | ||
| var isWip = /\b(wip|do not merge)\b/i.test(title) |
| owner: context.payload.repository.owner.login, | ||
| repo: context.payload.repository.name, | ||
| number: context.payload.pull_request.number | ||
| }) |
There was a problem hiding this comment.
you can simplify that a bit by using context.repo({number: context.payload.pull_request.number}) as below in the .createStatus call
|
oops I'm sorry I made a reivew and forgot to submit it. I see you made a few changes since then, I'll have another look Thanks for the review @HeeL 👍 |
|
I apologise in advanced for being overly diligent. Usually I'd accept the pull request already because it's great, then take the discussion to a follow up pull request, because merging is fun 🎉 The wip-bot is special because me and others use it for learning purpose, too, so I want to make double sure that it's good code and easy to read. I really like the idea by @HeeL. What I would like to avoid is to send a request for the commits with the title already contains a I gave it some thought and came up with this module.exports = handlePullRequestChange
async function handlePullRequestChange (robot, context) {
const {title, html_url, head} = context.payload.pull_request
const isWip = containsWIP(title) || await commitsContainWIP(context)
const status = isWip ? 'pending' : 'success'
console.log('Updating PR "%s" (%s): %s', title, html_url, status)
context.github.repos.createStatus(context.repo({
sha: head.sha,
state: status,
target_url: 'https://github.com/apps/wip',
description: isWip ? 'work in progress – do not merge!' : 'ready for review',
context: 'WIP'
}))
}
async function commitsContainWIP ({github, payload, repo}) {
const commits = await github.pullRequests.getCommits(repo({
number: payload.pull_request.number
})
return commits.data.map(element => element.commit.message).some(containsWIP)
}
function containsWIP (string) {
return /\b(wip|do not merge)\b/i.test(string)
}It's only a base of discussion. I'd love to hear what you think. Maybe we can make it even simpler while keeping it readable? Fun exercise in group refactoring :) |
|
No problem, I understand. I don't have enough experience in Node to write the best code, so this is an occasion to improve myself :) Your code is really understandable in my opinion, and I don't see ways to make it more concise. I executed your code on my machine, and I get a |
|
Hmm can't tell from the code, I can give this a try myself. For debugging, if you run this code locally, I recommend to use the Chrome Devtools Inspect. Put a Open chrome and it should open the devtools automatically. Now the execution should stop when the code gets to the function and you have the full chrome devtools available for debugging :) |
|
Thanks for the tip! I used VS Code and LocalTunnel instead to inspect variables, and looks like there is a problem with the strict mode Here is what I get: In detail what's in |
|
that's a weird error you get. What node version are you running the app in? Make sure it's Node 8 or 9 |
|
I use 8.9.4 on Windows 10 |
|
ah wait I know what this is, I run into it before. The For this code to work we can't pass |
|
Yep, and it works! 👌 Do I push that? |
|
yes please update the PR 👍 |
|
You welcome, it was a pleasure working on it! 🎉 |
|
I fear the deployment failed, I'm looking into it |
|
alright, all set, enjoy the new WIP :) |
|
Great! I was afraid I broke something 😅 |
| const status = isWip ? 'pending' : 'success' | ||
|
|
||
| console.log(`Updating PR "${title}" (${context.payload.pull_request.html_url}): ${status}`) | ||
| console.log('Updating PR "%s" (%s): %s', title, html_url, status) |
There was a problem hiding this comment.
Any particular reason for switching from ${template literals} ?
This PR is great btw. 👍 😄
There was a problem hiding this comment.
that was more by accident 😁 At some point during refactoring I had this
console.log('Updating PR "%s" (%s): %s',
context.payload.pull_request.title,
context.payload.pull_request.html_url,
context.payload.pull_request.status)Putting it all in one line made it less readable. Now that it's just console.log('Updating PR "%s" (%s): %s', title, html_url, status) we could as well have left it at template literals. Feel free to send a pull request if you like, I think template literals make it more readable :)
As asked in #10, this PR proposes to detect commits containing
wipin the pull request, and sets the commit status topendingif so.