Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expand and formalize contribution workflow a bit #65

Merged
merged 5 commits into from
Apr 4, 2019
Merged

Expand and formalize contribution workflow a bit #65

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e87ca65
Expand and formalize contribution workflow a bit
fitzgen Apr 1, 2019
064eb13
Fix typo: "from_fraw" -> "from_raw"
fitzgen Apr 2, 2019
fd04aa1
Clarify that if any team member has implementation concerns, those mu…
fitzgen Apr 2, 2019
674e3b4
Add comment describing how to re-render workflow graph
fitzgen Apr 2, 2019
8eb9b18
Add a note about purely internal bug fixes not needing design discussion
fitzgen Apr 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
147 changes: 116 additions & 31 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,12 @@ yourself.
- [Wasm Headless Browser Tests](#wasm-headless-browser-tests)
- [Non-Wasm Tests](#non-wasm-tests)
- [Formatting](#formatting)
- [Gloo Crate Checklist](#gloo-crate-checklist)
- [Designing APIs](#designing-apis)
- [Workflow](#workflow)
- [Proposing a Design](#proposing-a-design)
- [Design Checklist](#design-checklist)
- [Implementation and Feedback Cycle](#implementation-and-feedback-cycle)
- [Implementation Checklist](#implementation-checklist)
- [Team Members](#team-members)

<!-- END doctoc generated TOC please keep comment here to allow auto update -->

Expand Down Expand Up @@ -92,34 +96,65 @@ To (re)format the Gloo source code, run:
$ cargo fmt --all
```

## Gloo Crate Checklist
## Workflow

Here is a checklist that all Gloo utility crates should fulfill:
Designing APIs for Gloo, its utility crates, and interfaces between them takes a
lot of care. The design space is large, and there is a lot of prior art to
consider. When coming to consensus on a design, we use a simplified, informal
version of [our RFC process][rfcs], where we have design discussions inside the
Gloo issues tracker.
fitzgen marked this conversation as resolved.
Show resolved Hide resolved

* [ ] The crate should be named `gloo-foobar`, located at `crates/foobar`, and
re-exported from the umbrella crate like:
> Note: when fixing a bug in a semver-compatible way that doesn't add any new
> API surface (i.e. changes are purely internal) we can skip the design proposal
> part of this workflow, and jump straight to a pull request.

```rust
pub use gloo_foobar as foobar;
```
The graph below gives an overview of the workflow for proposing, designing,
implementing, and merging new crates and APIs into Gloo. Notably, we expect a
large amount of design discussion to happen up front in the issue thread for the
design proposal.

* [ ] The `authors` entry in `Cargo.toml` is "The Rust and WebAssembly Working
Group".
[![Graph showing the workflow of proposing, designing, and merging new crates and
APIs into Gloo](./new-design-workflow.png)](./new-design-workflow.png)

* [ ] Crate's public interface follows the [Rust API Guidelines][api-guidelines].
[rfcs]: https://github.com/rustwasm/rfcs

### Proposing a Design

* [ ] Uses [`unwrap_throw` and `expect_throw`][unwrap-throw] instead of normal `unwrap` and
`expect`.
Before writing pull requests, we should have a clear idea of what is required
for implementation. This means there should be a skeleton of the API in the form
of types and function/method signatures. We should have a clear idea of the
layers of APIs we are exposing, and how they are built upon one another.

Note that exploratory implementations outside of Gloo are encouraged during this
period to get a sense for the API's usage, but you shouldn't send a pull request
until the design has been accepted.

Before the design is accepted, at least two team members must have stated that
they are in favor of accepting the design in the issue thread.

[Here is an issue template you can use for proposing
designs.](https://github.com/rustwasm/gloo/issues/new?assignees=&labels=&template=propose_design.md&title=)

#### Design Checklist

Here is a checklist of some general design principles that Gloo crates and APIs
should follow:

* [ ] Crate's public interface follows the [Rust API Guidelines][api-guidelines].

* [ ] Callback-taking APIs are generic over `F: Fn(A) -> B` (or `FnMut` or
`FnOnce`) instead of taking `wasm_bindgen::Closure`s or
`js_sys::Function`s directly.

* [ ] If the API can be implemented as a Future / Stream, then it should first be implemented as a callback, with the callback API put into the `callback` submodule.
* [ ] If the API can be implemented as a Future / Stream, then it should first
be implemented as a callback, with the callback API put into the
`callback` submodule.

Then the Future / Stream should be implemented using the callback API, and should be put into the `future` or `stream` submodule.
Then the Future / Stream should be implemented using the callback API, and
should be put into the `future` or `stream` submodule.

Make sure that the callback and Future / Stream APIs properly support cancellation (if it is possible to do so).
Make sure that the callback and Future / Stream APIs properly support
cancellation (if it is possible to do so).

* [ ] Uses nice Rust-y types and interfaces instead of passing around untyped
`JsValue`s.
Expand All @@ -129,32 +164,82 @@ Here is a checklist that all Gloo utility crates should fulfill:
escape hatch for dropping down to raw `web_sys` bindings when an API isn't
fully supported by the crate yet.

* [ ] There is a loose hierarchy with "mid-level" APIs (which are essentially thin wrappers over the low-level APIs), and "high-level" APIs (which make more substantial changes).
Similar for `from_raw` constructors and `into_raw` conversion methods when
applicable.

* [ ] There is a loose hierarchy with "mid-level" APIs (which are essentially
thin wrappers over the low-level APIs), and "high-level" APIs (which make
more substantial changes).

As a general rule, the high-level APIs should be built on top of the
mid-level APIs, which in turn should be built on top of the low-level APIs
(e.g. `web_sys`)

As a general rule, the high-level APIs should be built on top of the mid-level APIs, which in turn should be built on top of the low-level APIs (e.g. `web_sys`)

There are exceptions to this, but they have to be carefully decided on a case-by-case basis.
There are exceptions to this, but they have to be carefully decided on a
case-by-case basis.

### Implementation and Feedback Cycle

Once we've accepted a design, we can move forward with implementation and
creating pull requests.

The implementation should generally be unsurprising, since we should have
already worked out most of the kinks during the earlier design discussions. If
there are significant new issues or concerns uncovered during implementation,
then these should be brought up in the design proposal discussion thread again,
and the evolved design reaffirmed with two team members signing off once
again.

If there are no new concerns uncovered, then the implementation just needs to be
checked over by at least one team member. They provide code review and feedback
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@samcday samcday Apr 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

on the implementation, then the feedback is addressed and pull request updated.
Once the pull request is in a good place and CI is passing, a team member may
approve the pull request and merge it into Gloo. If any team member raises
concerns with the implementation, they must be resolved before the pull request
is merged.

#### Implementation Checklist

Here is a checklist that all crate and API implementations in Gloo should
fulfill:

* [ ] The crate should be named `gloo-foobar`, located at `gloo/crates/foobar`,
and re-exported from the umbrella Gloo crate like:

```rust
// gloo/src/lib.rs

pub use gloo_foobar as foobar;
```

* [ ] The `authors` entry in `gloo/crates/foobar/Cargo.toml` is "The Rust and
WebAssembly Working Group".

* [ ] Uses [`unwrap_throw` and `expect_throw`][unwrap-throw] instead of normal
`unwrap` and `expect`.

* [ ] Headless browser and/or Node.js tests via `wasm-pack test`.

* [ ] Uses `#![deny(missing_docs, missing_debug_implementations)]`.

* [ ] Crate's root module documentation has at least one realistic example.

* [ ] Crate has at least a brief description of how to use it in the Gloo guide.
* [ ] Crate has at least a brief description of how to use it in the Gloo guide
(the `mdbook` located at `gloo/guide`).

[unwrap-throw]: https://docs.rs/wasm-bindgen/0.2.37/wasm_bindgen/trait.UnwrapThrowExt.html
[api-guidelines]: https://rust-lang-nursery.github.io/api-guidelines/

## Designing APIs
## Team Members

Designing APIs for Gloo, its utility crates, and interfaces between them takes a
lot of care. The design space is large, and there is a lot of prior art to
consider. When coming to consensus on a design, we use a simplified, informal
version of [our RFC process][rfcs], where we have design discussions inside the
Gloo issues tracker.
Team members sign off on design proposals and review pull requests to Gloo.

[Here is an issue template you can use for proposing
designs.](https://github.com/rustwasm/gloo/issues/new?assignees=&labels=&template=propose_design.md&title=)
* `@fitzgen`
* `@Pauan`
* `@rylev`
* `@yoshuawuyts`
* `@David-OConnor`

[rfcs]: https://github.com/rustwasm/rfcs
If you make a handful of significant contributions to Gloo and participate in
design proposals, then maybe you should be a team member! Think you or someone
else would be a good fit? Reach out to us!
98 changes: 98 additions & 0 deletions new-design-workflow.dot
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
// Re-render the PNG image with:
//
// dot -Tpng new-design-workflow.dot -o new-design-workflow.png

digraph {
fitzgen marked this conversation as resolved.
Show resolved Hide resolved
nodesep = 1.5;

// Nodes

start [shape = "point", style = "invis", width = 0];
proposed [label = <<b>Design Proposed</b>>, shape = "circle", style = "filled", fillcolor = "#c5def5"];
accepted [label = <<b>Design Accepted</b>>, shape = "circle", style = "filled", fillcolor = "#c5def5"];
closed [label = <<b>Closed</b>>, shape = "doublecircle", style = "filled", fillcolor = "#f48b9b"];

wip_impl [label = <<b>WIP Implementation</b>>, shape = "circle", style = "filled", fillcolor = "#c5def5"];
changes_needed [label = <<b>Changes Needed</b>>, shape = "circle", style = "filled", fillcolor = "#c5def5"];
merged [label = <<b>Merged into Gloo</b>>, shape = "doublecircle", style = "filled", fillcolor = "#0be27e"];

// Edges

start -> proposed [label = <
<table border="0">
<tr><td align="left" cellpadding="5"><b>📝 Propose API Design</b></td></tr>
<tr><td align="left">Explain motivation and use cases</td></tr>
<tr><td align="left">Alternatives and rationale</td></tr>
<tr><td align="left">Uses API proposal template</td></tr>
</table>
>];

proposed -> proposed [label = <
<table border="0">
<tr><td align="left" cellpadding="5"><b>💬 Design Discussion</b></td></tr>
<tr><td align="left">Determine public types and function signatures</td></tr>
<tr><td align="left">Are the abstractions zero-cost?</td></tr>
<tr><td align="left">Does it expose the onion layers?</td></tr>
<tr><td align="left">Work towards consensus</td></tr>
</table>
>];

proposed -> closed [label = <
<table border="0">
<tr><td align="left" cellpadding="5"><b>❌ Rejected</b></td></tr>
<tr><td align="left">Fatal flaw discovered</td></tr>
<tr><td align="left">Consensus this doesn't belong in Gloo</td></tr>
</table>
>];

proposed -> closed [label = <
<table border="0">
<tr><td align="left" cellpadding="5"><b>📆 Delayed</b></td></tr>
<tr><td align="left">Promising, but don't want to focus on it now</td></tr>
<tr><td align="left">Blocked on something else</td></tr>
</table>
>];

proposed -> accepted [label = <
<table border="0">
<tr><td align="left" cellpadding="5"><b>✔ Accepted</b></td></tr>
<tr><td align="left">Consensus we want this design</td></tr>
<tr><td align="left">Clear how we will implement it</td></tr>
<tr><td align="left">2 or more team members sign off</td></tr>
<tr><td align="left">0 team members register concerns</td></tr>
</table>
>];

accepted -> wip_impl [label = <
<table border="0">
<tr><td align="left" cellpadding="5"><b>🔨 Pull Request Created</b></td></tr>
<tr><td align="left">Adds new <font face="monospace">gloo_whatever</font> crate</td></tr>
<tr><td align="left">Adds tests/docs/examples</td></tr>
</table>
>];

wip_impl -> changes_needed [label = <
<table border="0">
<tr><td align="left" cellpadding="5"><b>🤔 Changes Requested</b></td></tr>
<tr><td align="left">Missing docs/examples/tests</td></tr>
<tr><td align="left">CI is failing</td></tr>
</table>
>];

changes_needed -> wip_impl [label = <
<table border="0">
<tr><td align="left" cellpadding="5"><b>😂 Feedback Addressed</b></td></tr>
<tr><td align="left">Tests/docs/examples fixed</td></tr>
<tr><td align="left">CI is green</td></tr>
</table>
>];

wip_impl -> merged [label = <
<table border="0">
<tr><td align="left" cellpadding="5"><b>🎉 Merge</b></td></tr>
<tr><td align="left">All crate checklist items are checked</td></tr>
<tr><td align="left">CI is green</td></tr>
<tr><td align="left">At least 1 team member approves and merges PR</td></tr>
</table>
>];
}
Binary file added new-design-workflow.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.