-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Foundations of API Design chapter #2994
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
(GitHub does not allow me to leave a comment on an empty file) |
src/idiomatic/foundations-api-design/meaningful-doc-comments.md
Outdated
Show resolved
Hide resolved
src/idiomatic/foundations-api-design/meaningful-doc-comments/avoid-redundancy.md
Outdated
Show resolved
Hide resolved
src/idiomatic/foundations-api-design/meaningful-doc-comments/avoid-redundancy.md
Outdated
Show resolved
Hide resolved
| ``` | ||
|
|
||
| <details> | ||
| - Motivation: Documentation that repeats name/signature information provides nothing new to the API user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - Motivation: Documentation that repeats name/signature information provides nothing new to the API user. | |
| - Motivation: Documentation that merely repeats name/signature information provides nothing new to the API user. |
| ``` | ||
|
|
||
| <details> | ||
| - Motivation: It can be easy to fall into a pattern of writing only for you, but most documentation is for people coming in with a different perspective. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - Motivation: It can be easy to fall into a pattern of writing only for you, but most documentation is for people coming in with a different perspective. | |
| - Background: The [curse of knowledge](https://en.wikipedia.org/wiki/Curse_of_knowledge) is a cognitive bias where experts assume that others have the same level of expertise and perspective. | |
| - Motivation: Your reader does not have the same level of expertise and the same perspective as you. Don't write for people like yourself, write for others. |
|
|
||
| ```rust | ||
| // TODO: What's a good illustration here? | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// expert writes for experts
/// Canonicalizes the MIR for the borrow checker.
///
/// This pass ensures that all borrows conform to the NLL-Polonius constraints
/// before we proceed to MIR-to-LLVM-IR translation.
pub fn canonicalize_mir(mir: &mut Mir) {
// ...
}
// expert writes for newcomers
/// Prepares the Mid-level IR (MIR) for borrow checking.
///
/// The borrow checker operates on a simplified, "canonical" form of the MIR.
/// This function performs that transformation. It is a prerequisite for the
/// final stages of code generation.
///
/// For more about Rust's intermediate representations, see the
/// [rustc-dev-guide](https://rustc-dev-guide.rust-lang.org/mir/index.html).
pub fn canonicalize_mir(mir: &mut Mir) {
// ...
}| much information. | ||
| - Always ask: Is this documentation making it difficult for the API user? Are | ||
| they able to quickly grasp what they need or find out where they could need | ||
| it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also say something like "Experts also read API level documentation. Doc comments might not be the right place to educate your audience about the basics of your domain. In that case, signpost and namedrop - divert people to long-form documentation."
|
|
||
| # Predictable API | ||
|
|
||
| Keep your APIs predictable through naming conventions and implementing common |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Keep your APIs predictable through naming conventions and implementing common | |
| Keep your APIs predictable by following naming conventions and implementing common |
Parallel sentence structure.
| Keep your APIs predictable through naming conventions and implementing common | ||
| traits. | ||
|
|
||
| <!-- TODO --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some speaker notes would be nice. What is the framing for this section?
gribozavr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(misclick, I meant to simply "comment", but clicked "approve"; for clarity switching to "request changes")
| # Naming Conventions | ||
|
|
||
| <details> | ||
| - One core component of readability and predictability is the way function names are composed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - One core component of readability and predictability is the way function names are composed. | |
| - One core component of API readability and predictability is the way function names are composed. |
| Rust's community developed naming conventions early, making them mostly | ||
| consistent in places like the standard library. | ||
|
|
||
| - Here we'll learn common components of rust method names, giving examples from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - Here we'll learn common components of rust method names, giving examples from | |
| - In this section we'll learn common components of Rust method names, giving examples from |
| like a domain-specific language and quickly understand the functionality and use | ||
| cases of a method. | ||
|
|
||
| Rust's community developed naming conventions early, making them mostly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Rust's community developed naming conventions early, making them mostly | |
| - Rust community developed naming conventions early, making them mostly |
| <details> | ||
| - One core component of readability and predictability is the way function names are composed. | ||
|
|
||
| A formal and consistently-applied naming convention lets developers treat names |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| A formal and consistently-applied naming convention lets developers treat names | |
| - Formal and consistently-applied naming conventions let developers treat names |
| @@ -0,0 +1,31 @@ | |||
| --- | |||
| minutes: 2 | |||
| --- | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are great slides about naming conventions, but I have a couple of comments. I have left the comments on the "get" and "push" slides, but please apply them everywhere. All slides in this section should follow the same structure.
| <details> | ||
| - Method for getting a reference-style value from an owned or borrowed value. | ||
|
|
||
| - Often used for getting something internal to a type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - Often used for getting something internal to a type. | |
| - The type implementing an "as" method should contain one primary piece of data that is being borrowed out. | |
| - The "as" naming convention does not work if the data type is an aggregate of many fields without an obvious primary one. Think about the call site: | |
| ```rust | |
| my_vec.as_ptr() // OK | |
| my_person.as_first_name() // does not read right, don't use "as_" | |
| my_person.first_name() // OK | |
| ``` | |
| - If you want to have two getters that you need to distinguish, one that returns first name by value, and another one that returns it by reference, use `_ref` suffix: | |
| ```rust | |
| impl Person { | |
| fn first_name(&self) -> String | |
| fn first_name_ref() -> &str | |
| fn first_name_mut() -> &mut String | |
| } | |
| ``` | |
"Something internal" is quite unspecific. Here's my attempt at making it concrete. The "as" pattern is not applicable in every case, we need to have a type that wraps one specific thing that is being borrowed out.
Given the number of code examples, could you make a separate slide for this point that has all the necessary code snippets on the slide?
| Slice::get // ? | ||
| Slice::get_unchecked_mut // ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Slice::get // ? | |
| Slice::get_unchecked_mut // ? | |
| slice::get // ? | |
| slice::get_unchecked_mut // ? |
| 2. What should we name these signatures? | ||
|
|
||
| ```rust,compile_fail | ||
| // What are the types for these methods? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // What are the types for these methods? | |
| // What are the types of these methods? |
| Option::is_some // ? | ||
| Slice::get // ? | ||
| Slice::get_unchecked_mut // ? | ||
| Option::as_ref // ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Option::as_ref // ? | |
| Option::as_ref // ? | |
| str::from_utf8_unchecked_mut // ? | |
| Rc::get_mut // ? | |
| Vec::dedup_by_key // ? |
| types of the functions should be. | ||
|
|
||
| - Go through the unnamed methods and brainstorm what names those methods should | ||
| have. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add expected answers to the speaker notes (for both Qs).
| PathBuf::with_extension | ||
| PathBuf::with_file_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two methods are actually defined on Path (they are callable through PathBuf because PathBuf: Deref<Target=Path>)
| @@ -0,0 +1,29 @@ | |||
| --- | |||
| Minutes: 2 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Minutes: 2 | |
| minutes: 2 |
| Prefix to a function that takes a borrowed value and creates an owned value | ||
|
|
||
| ```rust | ||
| String::to_owned // &str -> String (&str is not consumed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| String::to_owned // &str -> String (&str is not consumed) | |
| str::to_owned // &str -> String (&str is not consumed) |
| minutes: 10 | ||
| --- | ||
|
|
||
| Mini-exercise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Mini-exercise | |
| # Exercise |
| minutes: 10 | ||
| --- | ||
|
|
||
| Mini-exercise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename the file as well to remove "mini".
Co-authored-by: Dmitri Gribenko <gribozavr@gmail.com>
| - Traits are one of the most potent tools in the Rust language. The language and ecosystem expects you to use them, and so a big part of _predictability_ is what traits are implemented for a type! | ||
|
|
||
| - Traits should be liberally implemented on types you author, but there are | ||
| caveats! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this point Google's Rust style guide also leans on the distinction between library and application code (see the definition in another comment I left previously on this PR).
Specifically, we say this:
### Add trait implementations when they become necessary
As a general rule, low-level library code should anticipate users' needs
proactively, while application code should add trait impls as needed.We also provide guidance for implementing individual traits in library code, which should go into respective slides.
### Specific guidance for important traits {#important-traits}
* **Types with public fields**: Consider implementing traits implemented by
all subfields.
* **Error types**: Implement the `Copy` trait whenever possible. `Copy` types
are simpler to manage and propagate because they can be passed by value
without moving ownership. An example of a case where this is not possible is
if the error must contain non-`Copy` fields like a `String`.
* **`Copy`**: Implement whenever it is reasonably clear that the type will
continue to be `Copy` in the future.
* **`Clone`**: Most types should implement `Clone` unless they represent some
external resource.
* **`Default`**: Implement if the type implements `new()` or otherwise has a
default state. For context, see Clippy lint
[`new_without_default`](https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default).
* **`PartialEq`** / **`Eq`**: If you want users to use the `==` operator,
prefer implementing `PartialEq` over `Eq`. When you do implement `Eq`, be
aware of the additionally required reflexivity (details:
https://doc.rust-lang.org/std/cmp/trait.Eq.html).
* **`PartialOrd`** / **`Ord`**: If you want users to use the `<` or `>`
operators, prefer implementing `PartialOrd` over `Ord`. When you do
implement `Ord`, be aware that it requires total ordering (details:
https://doc.rust-lang.org/std/cmp/trait.Ord.html).
* **Map keys**
* If you think it is likely that a type will be used as a map key or
otherwise cached, implement `Hash` and `Eq`.
* If, in addition, the type has a clear total ordering, implement `Ord`
(and any traits it needs).
|
|
||
| "Write to string" trait, for debug purposes. | ||
|
|
||
| Derivable: ✅ When to implement: Almost always |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional that you put both of these things on the same line? (to save space?)
If yes, maybe add some space before "When" with characters for visual separation.
(on all slides)
| minutes: 10 | ||
| --- | ||
|
|
||
| PartialEq and Eq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| PartialEq and Eq | |
| # `PartialEq` and `Eq` |
Please apply to all slides: ensure there is a Markdown section title element, and the trait names are typeset using code font.
| - A type can't implement `Eq` without implementing `PartialEq`. | ||
|
|
||
| - Reminder: Partial means "there are invalid members of this set for this | ||
| function." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use more words to explain partial equality. "invalid" is a bit abstract and unclear, if one is not already familiar with this concept.
| function." | ||
|
|
||
| This doesn't mean that equality will panic, or that it returns a result, just | ||
| that there may be values that may not behave as you expect equality to behave. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please more directly say what equality laws those elements may break. I really want to hear the word "reflexivity" and a brief explanation of that.
| `PartialEq` exists to separate types like f32/f64 from types with Total | ||
| Equality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| `PartialEq` exists to separate types like f32/f64 from types with Total | |
| Equality. | |
| - Types which support only partial equality are relatively rare. Default to implementing `Eq` if you can. | |
| - `PartialEq` exists primarily to enable floating point types (`f32` and `f64`) to support a weak form of equality comparison through a trait, while enabling most types opt into the stronger guarantees of total equality through `Eq`. |
|
|
||
| Partial ordering & Total ordering. | ||
|
|
||
| Derivable: ✅ When to implement: Almost always. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really agree with the "almost always" guidance here. Many types don't have a natural order, and people should not be making up one unless it is really needed.
| /// Function from A to B // ❌ Redundant | ||
| fn a_to_b(a: A) -> B {...} | ||
| /// Connects to the database. // ❌ Lacks detail │ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// Connects to the database. // ❌ Lacks detail │ | |
| /// Connects to the database. // ❌ Lacks detail │ |
What's up with the vertical bar?
| - Application code has the opposite traits: it has few users, solves a specific\ | ||
| problem, and changes often. For application code elaborate documentation\ | ||
| quickly becomes outdated and misleading. It is also difficult to extract a\ | ||
| positive RoI from boilerplate docs even while they are up to date, because\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's up with the backslashes?
Includes most of chapter 1 of foundations of API Design for Idiomatic Rust. Brought in from the gdoc. Tests have not been run locally yet, formatting has.