Contribution guidelines#2726
Conversation
| AI is useful when communicating as a non-native English speaker. If you are using AI to edit your | ||
| comments for this purpose, please take the time to ensure it reflects your own voice and ideas. | ||
| When using AI for translation, we recommend writing in your native language and including the AI | ||
| translation in a quote block. |
There was a problem hiding this comment.
This is very nice, might be worth putting higher up in this section.
There was a problem hiding this comment.
Hmm, not sure -- I think at the bottom is probably okay for this since the other points seem more relevant to most consumers.
| - For large features, file an issue prior to starting work. | ||
| This means everyone can see what is in progress prior to a PR. | ||
| - Feel free to submit a PR even if the work is not totally finished, for early feedback. | ||
| - Prefer not to reference GitHub issue or PR numbers in commit messages. |
There was a problem hiding this comment.
Why is this? I've noticed consuming Quinn commits downstream that it is often a bit hard to find out the reasoning and motivation behind commits. Information that would be more readily available if issues and PRs were mentioned. Usually commit messages are not really part of the review process so they tend to be terse and rely on the PR context.
I'm not asking to change how you work, only curious how this practice came to be. Feel free to ignore :)
There was a problem hiding this comment.
The problem is that GitHub generates a lot of spam from these, so for example each rebase (as well as downstream merges) would generate a link in the linked issue, or, worse, a notification for the mentioned user.
| reviewed the PR yourself before submitting it for review to the maintainers. Do not copy responses | ||
| from the AI when replying to questions from maintainers. | ||
|
|
||
| This project requires a human in the loop who understands the work produced by AI. Autonomous |
There was a problem hiding this comment.
Someone I work with on other projects had questions about this part, in particular the second sentence, which seems fairly broad. Given that the constraints are pretty clearly enumerated in other paragraphs, perhaps this paragraph/sentence should be removed?
There was a problem hiding this comment.
That sounds good to me. As written, this paragraph might cause confusion about people using agentic workflows to draft code locally, which we don't intrinsically object to.
| reviewed the PR yourself before submitting it for review to the maintainers. Do not copy responses | ||
| from the AI when replying to questions from maintainers. | ||
|
|
||
| This project requires a human in the loop who understands the work produced by AI. Autonomous |
There was a problem hiding this comment.
That sounds good to me. As written, this paragraph might cause confusion about people using agentic workflows to draft code locally, which we don't intrinsically object to.
|
|
||
| * Avoid mixing refactoring and functional changes in the same commit if possible | ||
| * Make mechanical changes (like renaming or moving code around) in a separate commit | ||
| * Isolate updates to `Cargo.lock` in their own commits |
There was a problem hiding this comment.
Should we also mention amending/squashing/rebasing PR commits in response to feedback?
|
|
||
| This can be surprising to many engineers who are used to the bottom-up ordering | ||
| used in languages like Python, where items can have a run-time dependency on | ||
| other items defined in the same module. |
There was a problem hiding this comment.
Is this paragraph carrying any weight? It reads like it's just throwing shade at Python.
| generic, or easily confused between different crates. In this case we prefer to | ||
| import the symbol name under an alias, or if the parent module name is short, | ||
| using a one-level qualified path. E.g. for a crate with a local `Error` type, | ||
| prefer to `import std::error::Error as StdError`. |
| use alloc::vec::Vec; | ||
| ``` | ||
|
|
||
| We prefer to reference types and traits by an imported symbol name instead of |
There was a problem hiding this comment.
editorial:
| We prefer to reference types and traits by an imported symbol name instead of | |
| We prefer to reference items by an imported symbol name instead of |
| ### Exports | ||
|
|
||
| We prefer to export types under a single name, avoiding re-exporting types from | ||
| the top-level `lib.rs`. The exception to this are "paved path" exports that we |
There was a problem hiding this comment.
I don't think this is at all true for Quinn, and I'm pretty happy with that the way it is. We go to some effort exposing a flatter namespace than our internal code organization would naturally yield, regardless of how likely a typical downstream user is to need to name something. We do have some few public modules, but those are mostly limited to cases where all the types are very closely associated and might need a common prefix otherwise. The result is, IMO, substantially more ergonomic for users.
|
|
||
| ### Comments | ||
|
|
||
| Crate items should have descriptive doccomments on them. These are required for |
There was a problem hiding this comment.
editorial: I'm skeptical of "doccomments" as one word.
| is rare and most new types should be exported only from the module in which they | ||
| are defined. | ||
|
|
||
| ### Comments |
There was a problem hiding this comment.
Should we talk about non-doc comments?
|
|
||
| #### Type exhaustiveness | ||
|
|
||
| Public enums should be marked as _either_ `#[non_exhaustive]` or `#[allow(clippy::exhaustive_enums)]`. |
There was a problem hiding this comment.
Do we need to turn this lint on in CI?
| express that in the [library's defaults][defaults]. It is good to | ||
| encode our experience of TLS in this way, and we should not require that | ||
| users are experts in TLS to end up with a result that is working, | ||
| secure, stable, and performant. |
There was a problem hiding this comment.
Slightly odd elaboration for Quinn: while TLS is certainly a large part of what we must be wary of overwhelming the user with, there's much more beyond it. Maybe prefix "For example, it is good to ..."?
4 participants
This is based on rustls' guidelines, with an AI policy adapted from ripgrep's.