Detecting w i p commits

[Raul6469]

As asked in #10, this PR proposes to detect commits containing wip in the pull request, and sets the commit status to pending if so.

[HeeL]

I think it makes sense to re-use isWip from above, if you extract it as a function and pass string argument to it

[Raul6469]

Thanks for the review @HeeL ! I pushed a commit that follows your suggestion

[gr2m]

please use let :)

[gr2m]

you can simplify that a bit by using context.repo({number: context.payload.pull_request.number}) as below in the .createStatus call

[gr2m]

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 👍

[gr2m]

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

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 :)

[Raul6469]

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 TypeError: Cannot read property 'payload' of undefined in the commitsContainWIP function. However, it looks like payload is correctly defined in it. Do you have any idea why?

[gr2m]

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 debugger statement on top of the handlePullRequestChange function. Install the chrome extension, then start the probot app with

node --inspect probot run ./index.js

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 :)

[Raul6469]

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 repo

TypeError: 'caller', 'callee', and 'arguments' properties may not be accessed on strict mode functions or the arguments objects for calls to them

[gr2m]

that's a weird error you get. What node version are you running the app in? Make sure it's Node 8 or 9

[Raul6469]

I use 8.9.4 on Windows 10

[gr2m]

ah wait I know what this is, I run into it before. The context.repo method is not properly bound and is using the this keyword. It would be a nice PR to probot to fix that ;)

For this code to work we can't pass {github, payload, repo} but need to pass context instaed and then use context.repo etc. Could you give that a try please?

[Raul6469]

Yep, and it works! 👌 Do I push that?

[gr2m]

yes please update the PR 👍

[gr2m]

fabulous, thank you :)

[Raul6469]

You welcome, it was a pleasure working on it! 🎉

[gr2m]

I fear the deployment failed, I'm looking into it

[gr2m]

alright, all set, enjoy the new WIP :)

[Raul6469]

Great! I was afraid I broke something 😅

[anshumanv]

Any particular reason for switching from ${template literals} ?
This PR is great btw. 👍 😄

[gr2m]

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 :)

[anshumanv]

Sure @gr2m thanks! 🎉
Sending one. 👍