diff options
-rw-r--r-- | docs/dev/README.md | 105 |
1 files changed, 104 insertions, 1 deletions
diff --git a/docs/dev/README.md b/docs/dev/README.md index 65cc9fc12..1de5a2aab 100644 --- a/docs/dev/README.md +++ b/docs/dev/README.md | |||
@@ -30,7 +30,7 @@ https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Fwg-rls-2.2E0 | |||
30 | 30 | ||
31 | * [good-first-issue](https://github.com/rust-analyzer/rust-analyzer/labels/good%20first%20issue) | 31 | * [good-first-issue](https://github.com/rust-analyzer/rust-analyzer/labels/good%20first%20issue) |
32 | are good issues to get into the project. | 32 | are good issues to get into the project. |
33 | * [E-mentor](https://github.com/rust-analyzer/rust-analyzer/issues?q=is%3Aopen+is%3Aissue+label%3AE-mentor) | 33 | * [E-has-instructions](https://github.com/rust-analyzer/rust-analyzer/issues?q=is%3Aopen+is%3Aissue+label%3AE-has-instructions) |
34 | issues have links to the code in question and tests. | 34 | issues have links to the code in question and tests. |
35 | * [E-easy](https://github.com/rust-analyzer/rust-analyzer/issues?q=is%3Aopen+is%3Aissue+label%3AE-easy), | 35 | * [E-easy](https://github.com/rust-analyzer/rust-analyzer/issues?q=is%3Aopen+is%3Aissue+label%3AE-easy), |
36 | [E-medium](https://github.com/rust-analyzer/rust-analyzer/issues?q=is%3Aopen+is%3Aissue+label%3AE-medium), | 36 | [E-medium](https://github.com/rust-analyzer/rust-analyzer/issues?q=is%3Aopen+is%3Aissue+label%3AE-medium), |
@@ -117,6 +117,109 @@ Additionally, I use `cargo run --release -p rust-analyzer -- analysis-stats | |||
117 | path/to/some/rust/crate` to run a batch analysis. This is primarily useful for | 117 | path/to/some/rust/crate` to run a batch analysis. This is primarily useful for |
118 | performance optimizations, or for bug minimization. | 118 | performance optimizations, or for bug minimization. |
119 | 119 | ||
120 | # Code Style & Review Process | ||
121 | |||
122 | Our approach to "clean code" is two fold: | ||
123 | |||
124 | * We generally don't block PRs on style changes. | ||
125 | * At the same time, all code in rust-analyzer is constantly refactored. | ||
126 | |||
127 | 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. | ||
128 | Sending small cleanup PRs (like rename a single local variable) is encouraged. | ||
129 | |||
130 | ## Scale of Changes | ||
131 | |||
132 | Everyone knows that it's better to send small & focused pull requests. | ||
133 | The problem is, sometimes you *have* to, eg, rewrite the whole compiler, and that just doesn't fit into a set of isolated PRs. | ||
134 | |||
135 | The main thing too keep an eye on is the boundaries between various components. | ||
136 | There are three kinds of changes: | ||
137 | |||
138 | 1. Internals of a single component are changed. | ||
139 | Specifically, you don't change any `pub` items. | ||
140 | A good example here would be an addition of a new assist. | ||
141 | |||
142 | 2. API of a component is expanded. | ||
143 | Specifically, you add a new `pub` function which wasn't there before. | ||
144 | A good example here would be expansion of assist API, for example, to implement lazy assists or assists groups. | ||
145 | |||
146 | 3. A new dependency between components is introduced. | ||
147 | Specifically, you add a `pub use` reexport from another crate or you add a new line to `[dependencies]` section of `Cargo.toml`. | ||
148 | A good example here would be adding reference search capability to the assists crates. | ||
149 | |||
150 | For the first group, the change is generally merged as long as: | ||
151 | |||
152 | * it works for the happy case, | ||
153 | * it has tests, | ||
154 | * it doesn't panic for unhappy case. | ||
155 | |||
156 | For the second group, the change would be subjected to quite a bit of scrutiny and iteration. | ||
157 | The new API needs to be right (or at least easy to change later). | ||
158 | The actual implementation doesn't matter that much. | ||
159 | It's very important to minimize the amount of changed lines of code for changes of the second kind. | ||
160 | Often, you start doing change of the first kind, only to realise that you need to elevate to a change of the second kind. | ||
161 | In this case, we'll probably ask you to split API changes into a separate PR. | ||
162 | |||
163 | Changes of the third group should be pretty rare, so we don't specify any specific process for them. | ||
164 | That said, adding an innocent-looking `pub use` is a very simple way to break encapsulation, keep an eye on it! | ||
165 | |||
166 | Note: if you enjoyed this abstract hand-waving about boundaries, you might appreciate | ||
167 | https://www.tedinski.com/2018/02/06/system-boundaries.html | ||
168 | |||
169 | ## Order of Imports | ||
170 | |||
171 | We separate import groups with blank lines | ||
172 | |||
173 | ``` | ||
174 | mod x; | ||
175 | mod y; | ||
176 | |||
177 | use std::{ ... } | ||
178 | |||
179 | use crate_foo::{ ... } | ||
180 | use crate_bar::{ ... } | ||
181 | |||
182 | use crate::{} | ||
183 | |||
184 | use super::{} // but prefer `use crate::` | ||
185 | ``` | ||
186 | |||
187 | ## Order of Items | ||
188 | |||
189 | Optimize for the reader who sees the file for the first time, and wants to get the general idea about what's going on. | ||
190 | People read things from top to bottom, so place most important things first. | ||
191 | |||
192 | Specifically, if all items except one are private, always put the non-private item on top. | ||
193 | |||
194 | Put `struct`s and `enum`s first, functions and impls last. | ||
195 | |||
196 | Do | ||
197 | |||
198 | ``` | ||
199 | // Good | ||
200 | struct Foo { | ||
201 | bars: Vec<Bar> | ||
202 | } | ||
203 | |||
204 | struct Bar; | ||
205 | ``` | ||
206 | |||
207 | rather than | ||
208 | |||
209 | ``` | ||
210 | // Not as good | ||
211 | struct Bar; | ||
212 | |||
213 | struct Foo { | ||
214 | bars: Vec<Bar> | ||
215 | } | ||
216 | ``` | ||
217 | |||
218 | ## Documentation | ||
219 | |||
220 | For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines. | ||
221 | If the line is too long, you want to split the sentence in two :-) | ||
222 | |||
120 | # Logging | 223 | # Logging |
121 | 224 | ||
122 | Logging is done by both rust-analyzer and VS Code, so it might be tricky to | 225 | Logging is done by both rust-analyzer and VS Code, so it might be tricky to |