Skip to content

Commit

Permalink
Scale of changes
Browse files Browse the repository at this point in the history
  • Loading branch information
matklad committed Jun 2, 2020
1 parent 9d4cfcc commit c2570d0
Showing 1 changed file with 40 additions and 1 deletion.
41 changes: 40 additions & 1 deletion docs/dev/README.md
Expand Up @@ -117,7 +117,7 @@ Additionally, I use `cargo run --release -p rust-analyzer -- analysis-stats
path/to/some/rust/crate` to run a batch analysis. This is primarily useful for
performance optimizations, or for bug minimization.

# Code Style
# Code Style & Review Process

The approach to "clean code" is two fold:

Expand All @@ -127,6 +127,45 @@ The approach to "clean code" is two fold:
It is explicitly OK for reviewer to flag only some nits in the PR, and than send a follow up cleanup PR for things which are easier to explain by example, cc-ing the original author.
Sending small cleanup PRs (like rename a single local variable) is encouraged.

## Scale of Changes

Everyone knows that it's better to send small & focused pull requests.
The problem is, sometimes you *have* to, eg, rewrite the whole compiler, and that just doesn't fit into a set of isolated PRs.

The main thing too keep an eye on is the boundaries between various components.
There are three kinds of changes:

1. Internals of a single component are changed.
Specifically, you don't change any `pub` items.
A good example here would be an addition of a new assist.

2. API of a component is expanded.
Specifically, you add a new `pub` function which wasn't there before.
A good example here would be expansion of assist API, for example, to implement lazy assists or assists groups.

3. A new dependency between components is introduced.
Specifically, you add a `pub use` reexport from another crate or you add a new line to `[dependencies]` section of `Cargo.toml`.
A good example here would be adding reference search capability to the assists crates.

For the first group, the change is generally merged as long as:

* it works for the happy case,
* it has tests,
* it doesn't panic for unhappy case.

For the second group, the change would be subjected to a quite a bit of scrutiny and iteration.
The new API needs to be right (or at least easy to change later).
The actual implementation doesn't matter that much.
It's very important to minimize the amount of changed lines of code for changes of the second kind.
Often, you start doing change of the first kind, only to realise that you need to elevate to a change of the second kind.
In this case, we'll probably ask you to split API changes into a separate PR.

Changes of the third group should be pretty rare, so we don't specify any specific process for them.
That said, adding an innocent-looking `pub use` is a very simple way to break encapsulation, keep and eye on it!

Note: if you enjoyed this abstract hand-waving about boundaries, you might appreciate
https://www.tedinski.com/2018/02/06/system-boundaries.html

## Order of Imports

We separate import groups with blank lines
Expand Down

0 comments on commit c2570d0

Please sign in to comment.