Re: [rdf4j-dev] Using pull request approval more

[Håvard Ottestad <hmottestad@xxxxxxxxx>]

Hi Jeen and everyone else,

I’m good with this, and it will make things more explicit. 

It will affect turn around time though. You (Jeen) are the one to review 99% of my code. Sometimes I catch you in my evening or morning, but usually I will have to wait 12-24 hours for feedback at a minimum. 

I would like to propose that we don’t need to switch platforms. Pinging on GitHub should be enough. And furthermore I propose a Cart Blanche to merge if no one has responded in 48 hours. 

During the time that you were away, and James had taken over most of the coding, there weren’t really that many other people doing code review. Explicit approval wouldn’t have worked back then. Today we are in a better place wrt. resources. Though we are still far away from the bigger players who have multiple paid employees working on their open source projects. 

Fixing things after they are merged is also mostly fine. We shouldn’t aim for it, but I still say it’s mostly fine. We also used to do this a bit more frequently before we got the current GitHub actions setup to detect all test failures automatically and quickly. 

Håvard

On 21 Mar 2020, at 00:31, Jeen Broekstra <jeen.broekstra@xxxxxxxxx> wrote:



I've got a request: could we try and use explicit pull request approval (the green tick) a bit more to indicate that you are ok with a PR and/or have no objections to it being merged? And conversely, can we try and discipline ourselves to not actually merge a PR until we've received that green tick from at least one other committer, and from everyone who has previously provided review comments on the PR?

The reason I ask is that I often find myself uncertain whether people are fine with the changes I propose and/or how I address their PR feedback, and on the other side I've been caught out a few times where a pull request was merged after I had given some feedback, but before I was really ready to say "looks good to me". When I give feedback but no green approval tick, it usually means that I expect my feedback to be addressed before we can agree to merge.

Not that I think we've merged loads of things we shouldn't have, but I'd like it if we had a bit more explicit community scrutiny in place.

I'm not really in favor of putting full branch protections in place to enforce this, but I'd really appreciate it if we could be a bit more disciplined about this.

Rule of thumb: giving green tick of approval does not mean "I've fully tested this so if it breaks it's my fault". It simply means "I've had a look, seems good,and I have no objections to this being merged". 

Cheers,

Jeen

PS if you're waiting for a green tick on a PR and you're not getting, don't be shy about pinging people with a comment or (if that doesn't work) an e-mail.

_______________________________________________
rdf4j-dev mailing list
rdf4j-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/rdf4j-dev