Expand and formalize contribution workflow a bit

[fitzgen]

This lays out / formalizes the workflow I've had in my head for designing APIs, implementing them, and contributing to Gloo. Happy to use this as a starting point and modify it however. I figured having the graph visualization would be good for getting the idea across quickly.

What do you think?

cc @Pauan @rylev @yoshuawuyts @David-OConnor @samcday

[fitzgen]

Direct link to visualization: https://raw.githubusercontent.com/rustwasm/gloo/e87ca651b6e5c97574935b21940c01beb0221359/new-design-workflow.png

[Pauan]

I don't think it will be a problem, but I'd like to note that this does allow for one team member to approve the pull request while another disapproves.

And this also allows for the team member who approves the pull request to be different from the team members who OKed the design.

[samcday]

but I'd like to note that this does allow for one team member to approve the pull request while another disapproves.

I didn't get that impression. You only get to the "Merge" state if all pending feedback has been addressed. So if a team member says "please don't merge this until a test for foo has been written", that needs to be done before a different team member could approve and merge.

I guess it doesn't handle the (in my mind unlikely) scenario in which a team member is generally concerned about the PR overall. Like if they felt the implementation was just going in completely the wrong direction or something. But even in those cases, I would make the case that nobody should be saying "I disapprove of these changes!" without some concrete reason why. And those concrete reasons fall into the Changes Requested state.

[Pauan]

I was thinking more about the situation with team members being away for a while (or not having enough time to even look at the design).

Like I said though, I don't think it'll be a problem in practice. Just something to keep in mind.

[fitzgen]

I added a note that if any team member has concerns with an implementation, then that must be resolved before merging. I suspect that most of this stuff will already be worked out during the design proposal phase, though.

[Pauan]

Oh geez, I just wanted to leave comments, but GitHub instead treated it as a review.

So here's the actual review: I think this all looks great! The graph is really helpful.

[samcday]

This is great, thanks a lot for doing this.

[yoshuawuyts]

Digging this!

I'm kind of curious about a "recommendations section" for the last mile improvements, in particular:

#![forbid(unsafe_code, future_incompatible, rust_2018_idioms)]
#![deny(missing_debug_implementations, nonstandard_style)]
#![warn(missing_docs, missing_doc_code_examples)]

But that's in no way blocking, and at least somewhat subjective. Very happy with this patch!

[fitzgen]

I'm kind of curious about a "recommendations section" for the last mile improvements

We have some of these now (debug impls and docs). I think it is worth spinning the others off into another issue for discussion (since this PR is mostly about the workflow / rules of what constitutes acceptance of a design)

[rylev]

🎉Looks great!

[fitzgen]

Ok, once CI comes back green I'll merge this, since it seems like we all have general agreement that this is the direction we want to move in. As we work out the kinks in this workflow we can definitely adjust things and send follow up PRs as necessary.

Thanks everyone!