doc: document best practices for traits as part of style guide - #1618
Conversation
|
GitHub Actions
Inactive
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1618 +/- ##
==========================================
- Coverage 79.91% 79.91% -0.01%
==========================================
Files 461 461
Lines 80120 80120
Branches 80120 80120
==========================================
- Hits 64025 64024 -1
Misses 13887 13887
- Partials 2208 2209 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
GitHub Actions
Failure
GitHub Actions
Failure
anaPerezGhiglia
left a comment
anaPerezGhiglia
left a comment
There was a problem hiding this comment.
The CI is failing, and I left some suggestions that are worth considering before merging, but I'm approving it since is just a doc entry, to avoid blocking the merge
|
|
||
| When defining traits, try to keep traits composable by only defining the generic types, associated types and/or functions needed for a specific use case. | ||
|
|
||
| Furthermore, avoid overconstraining traits by only requiring: |
There was a problem hiding this comment.
This sentence is not easy to read. Is not clear until you read both bulletpoints which way is intended, since it can be interpreted both ways:
- you are overconstraining when you only require (which should be avoided)
- you should avoid overconstraining by requiring only the listed elements
Perhaps change it to
| Furthermore, avoid overconstraining traits by only requiring: | |
| Furthermore, avoid overconstraining traits. Require only: |
| When a user is calling a function or using a type with trait bounds, they will have to satisfy all the constraints of the trait bounds. | ||
| By using the least constraining option, you can reduce the number of constraints that need to be satisfied at usage sites; thus making the function more reusable and composable. | ||
|
|
||
| Moreover, this limits the number of obligations the trait solver has to discharge and—because well-formedness obligations feed region inference—the number of lifetime constraints that come with them. | ||
| Historically, we have run into problems where an unrelated trait bound resulted in an `outlives` requirement and the error blamed a lifetime that was in fact valid. |
There was a problem hiding this comment.
this is almost a copy-paste from "Defining traits" section. I think it can be unified. If you prefer keeping a Rationale subsection for each section, then I would suggest editing them so the reader can spot where the difference is
|
by the way, thanks for putting this together! 🙇 |
GitHub Actions
Inactive
`prettier --check "**/*.md"` runs in CI with `proseWrap: "never"`, so the hard-wrapped paragraphs and `*emphasis*` failed the lint.
"avoid overconstraining traits by only requiring" parsed two ways: the list could read as the cause of overconstraining or as the permitted set. Split it into an imperative and its object.
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Inactive
GitHub Actions
Error
GitHub Actions
Error
The rationales for defining traits and for using trait bounds argued the same point in near-identical wording. Both rules now live under a `Traits` heading that carries a single rationale, which also picks up the details each copy stated alone.
Also renames the second, differently defined `FullChainSpec` example, which collided with the first now that both sit under one heading.
In `trait A: B`, B is A's supertrait and A is B's subtrait. The guide had the two reversed, including in the derive-ordering rule, whose own example lists supertraits first.
b38e44d to
7a54f12
Compare
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
2 participants
Adds a section to the book's style guide explaining best practices and rules for using
traits that are implicit in the existing design of (most of) the multi-chain architecture.