Skip to content

Commit

Permalink
doc: update contributing and coding style
Browse files Browse the repository at this point in the history
* Update `CONTRIBUTING.md`
* Add `CODE_STYLE.md`
  • Loading branch information
yukibtc committed Apr 5, 2024
1 parent 9da9ab5 commit c9456f8
Show file tree
Hide file tree
Showing 2 changed files with 256 additions and 52 deletions.
187 changes: 187 additions & 0 deletions CODE_STYLE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
# Code style

This is a description of a coding style that every contributor **must** follow.
Please, read the whole document before you start pushing code.

## Generics

All trait bounds should be written in `where`:

```rust
// GOOD
pub fn new<N, T, P, E>(user_id: i32, name: N, title: T, png_sticker: P, emojis: E) -> Self
where
N: Into<String>,
T: Into<String>,
P: Into<InputFile>,
E: Into<String>,
{ ... }

// BAD
pub fn new<N: Into<String>,
T: Into<String>,
P: Into<InputFile>,
E: Into<String>>
(user_id: i32, name: N, title: T, png_sticker: P, emojis: E) -> Self { ... }
```

```rust
// GOOD
impl<T> Trait for Wrap<T>
where
T: Trait
{ ... }

// BAD
impl<T: Trait> Trait for Wrap<T> { ... }
```

## Use `Self` where possible

When referring to the type for which block is implemented, prefer using `Self`, rather than the name of the type:

```rust
impl ErrorKind {
// GOOD
fn print(&self) {
Self::Io => println!("Io"),
Self::Network => println!("Network"),
Self::Json => println!("Json"),
}

// BAD
fn print(&self) {
ErrorKind::Io => println!("Io"),
ErrorKind::Network => println!("Network"),
ErrorKind::Json => println!("Json"),
}
}
```

```rust
impl<'a> AnswerCallbackQuery<'a> {
// GOOD
fn new<C>(bot: &'a Bot, callback_query_id: C) -> Self
where
C: Into<String>,
{ ... }

// BAD
fn new<C>(bot: &'a Bot, callback_query_id: C) -> AnswerCallbackQuery<'a>
where
C: Into<String>,
{ ... }
}
```

**Rationale:** `Self` is generally shorter and it's easier to copy-paste code or rename the type.

## Deriving traits

Derive `Debug`, `Clone`, `Copy`, `PartialEq`, `Eq` and `Hash` for public types when possible (in this order).

**Rationale:** these traits can be useful for users and can be implemented for most types.

Derive `Default` when there is a reasonable default value for the type.

## Full paths for logging

Always write `tracing::<op>!(...)` instead of importing `use tracing::<op>;` and invoking `<op>!(...)`.

```rust
// GOOD
tracing::warn!("Everything is on fire");

// BAD
use tracing::warn;

warn!("Everything is on fire");
```

**Rationale:**
- Less polluted import blocks
- Uniformity

## `&str` -> `String` conversion

Prefer using `.to_string()` or `.to_owned()`, rather than `.into()`, `String::from`, etc.

**Rationale:** uniformity, intent clarity.

## Order of imports

```rust
// First core, alloc and/or std
use core::fmt;
use std::{...};

// Second, external crates (both crates.io crates and other rust-analyzer crates).
use crate_foo::{ ... };
use crate_bar::{ ... };

// If applicable, the current sub-modules
mod x;
mod y;

// Finally, the internal crate modules and submodules
use crate::{};
use super::{};

// Re-exports are treated as item definitions rather than imports, so they go
// after imports and modules. Use them sparingly.
pub use crate::x::Z;
```

## Import Style

When implementing traits from `core::fmt`/`std::fmt` import the module:

```rust
// GOOD
use core::fmt;

impl fmt::Display for RenameError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { .. }
}

// BAD
impl core::fmt::Display for RenameError {
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { .. }
}
```

## If-let

Avoid the `if let ... { } else { }` construct if possible, use `match` instead:

```rust
// GOOD
match ctx.expected_type.as_ref() {
Some(expected_type) => completion_ty == expected_type && !expected_type.is_unit(),
None => false,
}

// BAD
if let Some(expected_type) = ctx.expected_type.as_ref() {
completion_ty == expected_type && !expected_type.is_unit()
} else {
false
}
```

Use `if let ... { }` when a match arm is intentionally empty:

```rust
// GOOD
if let Some(expected_type) = this.as_ref() {
// Handle it
}

// BAD
match this.as_ref() {
Some(expected_type) => {
// Handle it
},
None => (),
}
```
121 changes: 69 additions & 52 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,36 +2,81 @@

## Contribution Workflow

The codebase is maintained using the "contributor workflow" where everyone
without exception contributes patch proposals using "pull requests". This
facilitates social contribution, easy testing and peer review.
To contribute a patch:

To contribute a patch, the workflow is a as follows:
* Fork Repository
* Create topic branch
* Commit patches (PR, emails, ...)

1. Fork Repository
2. Create topic branch
3. Commit patches
In general commits should be atomic and diffs **should be easy to read**.

In general commits should be atomic and diffs should be easy to read.
For this reason do not mix any formatting fixes or code moves with actual code
changes. Further, each commit, individually, should compile and pass tests, in
order to ensure git bisect and other automated tools function properly.
## Code style guide

When adding a new feature, thought must be given to the long term technical
debt.
Every new feature should be covered by unit tests where possible.
Before writing code, please read [our code style](./CODE_STYLE.md).

When refactoring, structure your PR to make it easy to review and don't
hesitate to split it into multiple small, focused PRs.
## Commit format

Commits should cover both the issue fixed and the solution's rationale.
These [guidelines](https://chris.beams.io/posts/git-commit/) should be kept in mind.
The commit **must** be formatted in the following way:

To facilitate communication with other contributors, the project is making use
of GitHub's "assignee" field. First check that no one is assigned and then
comment suggesting that you're working on it. If someone is already assigned,
don't hesitate to ask if the assigned party or previous commenters are still
working on it if it has been awhile.
```
<context>: <short descriptrion>
<optional description explaining better what happened in the commit>
```

If applicable, link the `issue`/`PR` to be closed with:

* Closes <url>
* Fixes <url>

The `context` name should be:

* `nostr` if changes are related to the main Rust `nostr` crate (or `protocol`?)
* `sdk`, `cli`, `pool`, `signer`, `nwc` and so on for the others Rust crates (so basically remove the `nostr-` prefix)
* `ffi(<name>)` for `UniFFI` and `js(<name>)` for `JavaScript` bindings (follow same above rules for the `<name>`)
* `book` if changes are related to the `book`
* `doc` for the `.md` files (except for `CHANGELOG.md`?)

Anything that haven't a specific context, can be left without the `<context>:` prefix (ex. change to main `justfile` commands, change to `CHANGELOG.md`?)

### Examples

```
nostr: add NIP32 support
Added kinds, tags and EventBuilder constructors to support NIP32.
Closes https://<domain>.com/rust-nostr/nostr/issue/1234
```

```
pool: fix connection issues
Long description...
Fixes https://<domain>.com/rust-nostr/nostr/issue/5612
```

```
nwc: add `pay_multiple_invoices` support
Closes https://<domain>.com/rust-nostr/nostr/issue/2222
```

```
ffi(nostr): add `EventBuilder::mute_list`
```

```
ffi(sdk): add `AbortHandle`
* Return `AbortHandle` in `Client::handle_notifications`
* Another change...
```

```
js(sdk): replace log `file path` with `module path`
```

## Deprecation policy

Expand All @@ -51,34 +96,6 @@ and send a follow-up to remove it as part of the next release cycle.

Usage of `.unwrap()` or `.expect("...")` methods is allowed **only** in `examples` or `tests`.

## Peer review

Anyone may participate in peer review which is expressed by comments in the
pull request. Typically reviewers will review the code for obvious errors, as
well as test out the patch set and opine on the technical merits of the patch.
PR should be reviewed first on the conceptual level before focusing on code
style or grammar fixes.

### Terminology

Concept ACK - Agree with the idea and overall direction, but haven't reviewed the code changes or tested them.

utACK (untested ACK) - Reviewed and agree with the code changes but haven't actually tested them.

tACK (tested ACK) - Reviewed the code changes and have verified the functionality or bug fix.

ACK - A loose ACK can be confusing. It's best to avoid them unless it's a documentation/comment only change in which case there is nothing to test/verify; therefore the tested/untested distinction is not there.

NACK - Disagree with the code changes/concept. Should be accompanied by an explanation.

## Coding Conventions

Use `just check` to format code and check the code before committing.
This is also enforced by the CI.

## Going further

You may be interested by Jon Atacks guide on [How to review Bitcoin Core PRs](https://github.com/jonatack/bitcoin-development/blob/master/how-to-review-bitcoin-core-prs.md)
and [How to make Bitcoin Core PRs](https://github.com/jonatack/bitcoin-development/blob/master/how-to-make-bitcoin-core-prs.md).
While there are differences between the projects in terms of context and
maturity, many of the suggestions offered apply to this project.
Use `just precommit` or `just check` to format and check the code before committing. This is also enforced by the CI.

0 comments on commit c9456f8

Please sign in to comment.