Skip to content

Commit 4aad8fb

Browse files
committed
docs: move style guide into a separate document
Code contribution guidelines are rearranged into a list of steps to follow.
1 parent 9640f92 commit 4aad8fb

File tree

2 files changed

+189
-155
lines changed

2 files changed

+189
-155
lines changed

CONTRIBUTING.md

Lines changed: 91 additions & 155 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
1-
# Contributing guidelines
1+
# Contributing to Delta Chat
22

3-
## Reporting bugs
3+
## Bug reports
44

55
If you found a bug, [report it on GitHub](https://github.com/deltachat/deltachat-core-rust/issues).
66
If the bug you found is specific to
@@ -9,178 +9,114 @@ If the bug you found is specific to
99
[Desktop](https://github.com/deltachat/deltachat-desktop/issues),
1010
report it to the corresponding repository.
1111

12-
## Proposing features
12+
## Feature proposals
1313

1414
If you have a feature request, create a new topic on the [forum](https://support.delta.chat/).
1515

16-
## Contributing code
16+
## Code contributions
1717

18-
If you want to contribute a code, [open a Pull Request](https://github.com/deltachat/deltachat-core-rust/pulls).
18+
If you want to contribute a code, follow this guide.
1919

20-
If you have write access to the repository,
21-
push a branch named `<username>/<feature>`
22-
so it is clear who is responsible for the branch,
23-
and open a PR proposing to merge the change.
24-
Otherwise fork the repository and create a branch in your fork.
20+
1. **Select an issue to work on.**
2521

26-
You can find the list of good first issues
27-
and a link to this guide
28-
on the contributing page: <https://github.com/deltachat/deltachat-core-rust/contribute>
22+
If you have an write access to the repository, assign the issue to yourself.
23+
Otherwise state in the comment that you are going to work on the issue
24+
to avoid duplicate work.
2925

30-
### Coding conventions
31-
32-
We format the code using `rustfmt`. Run `cargo fmt` prior to committing the code.
33-
Run `scripts/clippy.sh` to check the code for common mistakes with [Clippy].
34-
35-
### SQL
36-
37-
Multi-line SQL statements should be formatted using string literals,
38-
for example
39-
```
40-
sql.execute(
41-
"CREATE TABLE messages (
42-
id INTEGER PRIMARY KEY AUTOINCREMENT,
43-
text TEXT DEFAULT '' NOT NULL -- message text
44-
) STRICT",
45-
)
46-
.await?;
47-
```
48-
49-
Do not use macros like [`concat!`](https://doc.rust-lang.org/std/macro.concat.html)
50-
or [`indoc!](https://docs.rs/indoc).
51-
Do not escape newlines like this:
52-
```
53-
sql.execute(
54-
"CREATE TABLE messages ( \
55-
id INTEGER PRIMARY KEY AUTOINCREMENT, \
56-
text TEXT DEFAULT '' NOT NULL \
57-
) STRICT",
58-
)
59-
.await?;
60-
```
61-
Escaping newlines
62-
is prone to errors like this if space before backslash is missing:
63-
```
64-
"SELECT foo\
65-
FROM bar"
66-
```
67-
Literal above results in `SELECT fooFROM bar` string.
68-
This style also does not allow using `--` comments.
69-
70-
---
71-
72-
Declare new SQL tables with [`STRICT`](https://sqlite.org/stricttables.html) keyword
73-
to make SQLite check column types.
74-
75-
Declare primary keys with [`AUTOINCREMENT`](https://www.sqlite.org/autoinc.html) keyword.
76-
This avoids reuse of the row IDs and can avoid dangerous bugs
77-
like forwarding wrong message because the message was deleted
78-
and another message took its row ID.
79-
80-
Declare all new columns as `NOT NULL`
81-
and set the `DEFAULT` value if it is optional so the column can be skipped in `INSERT` statements.
82-
Dealing with `NULL` values both in SQL and in Rust is tricky and we try to avoid it.
83-
If column is already declared without `NOT NULL`, use `IFNULL` function to provide default value when selecting it.
84-
Use `HAVING COUNT(*) > 0` clause
85-
to [prevent aggregate functions such as `MIN` and `MAX` from returning `NULL`](https://stackoverflow.com/questions/66527856/aggregate-functions-max-etc-return-null-instead-of-no-rows).
86-
87-
Don't delete unused columns too early, but maybe after several months/releases, unused columns are
88-
still used by older versions, so deleting them breaks downgrading the core or importing a backup in
89-
an older version. Also don't change the column type, consider adding a new column with another name
90-
instead. Finally, never change column semantics, this is especially dangerous because the `STRICT`
91-
keyword doesn't help here.
92-
93-
### Commit messages
94-
95-
Commit messages follow the [Conventional Commits] notation.
96-
We use [git-cliff] to generate the changelog from commit messages before the release.
97-
98-
With **`git cliff --unreleased`**, you can check how the changelog entry for your commit will look.
99-
100-
The following prefix types are used:
101-
- `feat`: Features, e.g. "feat: Pause IO for BackupProvider". If you are unsure what's the category of your commit, you can often just use `feat`.
102-
- `fix`: Bug fixes, e.g. "fix: delete `smtp` rows when message sending is cancelled"
103-
- `api`: API changes, e.g. "api(rust): add `get_msg_read_receipts(context, msg_id)`"
104-
- `refactor`: Refactorings, e.g. "refactor: iterate over `msg_ids` without `.iter()`"
105-
- `perf`: Performance improvements, e.g. "perf: improve SQLite performance with `PRAGMA synchronous=normal`"
106-
- `test`: Test changes and improvements to the testing framework.
107-
- `build`: Build system and tool configuration changes, e.g. "build(git-cliff): put "ci" commits into "CI" section of changelog"
108-
- `ci`: CI configuration changes, e.g. "ci: limit artifact retention time for `libdeltachat.a` to 1 day"
109-
- `docs`: Documentation changes, e.g. "docs: add contributing guidelines"
110-
- `chore`: miscellaneous tasks, e.g. "chore: add `.DS_Store` to `.gitignore`"
111-
112-
Release preparation commits are marked as "chore(release): prepare for vX.Y.Z".
113-
114-
If you intend to squash merge the PR from the web interface,
115-
make sure the PR title follows the conventional commits notation
116-
as it will end up being a commit title.
117-
Otherwise make sure each commit title follows the conventional commit notation.
118-
119-
#### Breaking Changes
120-
121-
Use a `!` to mark breaking changes, e.g. "api!: Remove `dc_chat_can_send`".
122-
123-
Alternatively, breaking changes can go into the commit description, e.g.:
124-
125-
```
126-
fix: Fix race condition and db corruption when a message was received during backup
127-
128-
BREAKING CHANGE: You have to call `dc_stop_io()`/`dc_start_io()` before/after `dc_imex(DC_IMEX_EXPORT_BACKUP)`
129-
```
130-
131-
#### Multiple Changes in one PR
132-
133-
If you have multiple changes in one PR, create multiple conventional commits, and then do a rebase merge. Otherwise, you should usually do a squash merge.
134-
135-
[Clippy]: https://doc.rust-lang.org/clippy/
136-
[Conventional Commits]: https://www.conventionalcommits.org/
137-
[git-cliff]: https://git-cliff.org/
26+
If the issue does not exist yet, create it first.
27+
28+
2. **Write the code.**
29+
30+
Follow the [coding conventions](STYLE.md) when writing the code.
31+
32+
3. **Commit the code.**
33+
34+
If you have write access to the repository,
35+
push a branch named `<username>/<feature>`
36+
so it is clear who is responsible for the branch,
37+
and open a PR proposing to merge the change.
38+
Otherwise fork the repository and create a branch in your fork.
39+
40+
Commit messages follow the [Conventional Commits] notation.
41+
We use [git-cliff] to generate the changelog from commit messages before the release.
42+
43+
With **`git cliff --unreleased`**, you can check how the changelog entry for your commit will look.
44+
45+
The following prefix types are used:
46+
- `feat`: Features, e.g. "feat: Pause IO for BackupProvider". If you are unsure what's the category of your commit, you can often just use `feat`.
47+
- `fix`: Bug fixes, e.g. "fix: delete `smtp` rows when message sending is cancelled"
48+
- `api`: API changes, e.g. "api(rust): add `get_msg_read_receipts(context, msg_id)`"
49+
- `refactor`: Refactorings, e.g. "refactor: iterate over `msg_ids` without `.iter()`"
50+
- `perf`: Performance improvements, e.g. "perf: improve SQLite performance with `PRAGMA synchronous=normal`"
51+
- `test`: Test changes and improvements to the testing framework.
52+
- `build`: Build system and tool configuration changes, e.g. "build(git-cliff): put "ci" commits into "CI" section of changelog"
53+
- `ci`: CI configuration changes, e.g. "ci: limit artifact retention time for `libdeltachat.a` to 1 day"
54+
- `docs`: Documentation changes, e.g. "docs: add contributing guidelines"
55+
- `chore`: miscellaneous tasks, e.g. "chore: add `.DS_Store` to `.gitignore`"
56+
57+
Release preparation commits are marked as "chore(release): prepare for X.Y.Z"
58+
as described in [releasing guide](RELEASE.md).
59+
60+
Use a `!` to mark breaking changes, e.g. "api!: Remove `dc_chat_can_send`".
61+
62+
Alternatively, breaking changes can go into the commit description, e.g.:
13863

139-
### Errors
64+
```
65+
fix: Fix race condition and db corruption when a message was received during backup
14066
141-
Delta Chat core mostly uses [`anyhow`](https://docs.rs/anyhow/) errors.
142-
When using [`Context`](https://docs.rs/anyhow/latest/anyhow/trait.Context.html),
143-
capitalize it but do not add a full stop as the contexts will be separated by `:`.
144-
For example:
145-
```
146-
.with_context(|| format!("Unable to trash message {msg_id}"))
147-
```
67+
BREAKING CHANGE: You have to call `dc_stop_io()`/`dc_start_io()` before/after `dc_imex(DC_IMEX_EXPORT_BACKUP)`
68+
```
14869

149-
All errors should be handled in one of these ways:
150-
- With `if let Err() =` (incl. logging them into `warn!()`/`err!()`).
151-
- With `.log_err().ok()`.
152-
- Bubbled up with `?`.
70+
4. [**Open a Pull Request**](https://github.com/deltachat/deltachat-core-rust/pulls).
15371

154-
`backtrace` feature is enabled for `anyhow` crate
155-
and `debug = 1` option is set in the test profile.
156-
This allows to run `RUST_BACKTRACE=1 cargo test`
157-
and get a backtrace with line numbers in resultified tests
158-
which return `anyhow::Result`.
72+
Refer to the corresponding issue.
15973

160-
### Logging
74+
If you intend to squash merge the PR from the web interface,
75+
make sure the PR title follows the conventional commits notation
76+
as it will end up being a commit title.
77+
Otherwise make sure each commit title follows the conventional commit notation.
16178

162-
For logging, use `info!`, `warn!` and `error!` macros.
163-
Log messages should be capitalized and have a full stop in the end. For example:
164-
```
165-
info!(context, "Ignoring addition of {added_addr:?} to {chat_id}.");
166-
```
79+
5. **Make sure all CI checks succeed.**
16780

168-
Format anyhow errors with `{:#}` to print all the contexts like this:
169-
```
170-
error!(context, "Failed to set selfavatar timestamp: {err:#}.");
171-
```
81+
CI runs the tests and checks code formatting.
17282

173-
### Reviewing
83+
While it is running, self-review your PR to make sure all the changes you expect are there
84+
and there are no accidentally commited unrelated changes and files.
17485

175-
Once a PR has an approval and passes CI, it can be merged.
86+
Push the necessary fixup commits or force-push to your branch if needed.
17687

177-
PRs from a branch created in the main repository, i.e. authored by those who have write access, are merged by their authors.
178-
This is to ensure that PRs are merged as intended by the author,
179-
e.g. as a squash merge, by rebasing from the web interface or manually from the command line.
88+
6. **Ask for review.**
18089

181-
If you do not have access to the repository and created a PR from a fork,
182-
ask the maintainers to merge the PR and say how it should be merged.
90+
Use built-in GitHub feature to request a review from suggested reviewers.
91+
92+
If you do not have write access to the repository, ask for review in the comments.
93+
94+
7. **Merge the PR.**
95+
96+
Once a PR has an approval and passes CI, it can be merged.
97+
98+
PRs from a branch created in the main repository,
99+
i.e. authored by those who have write access, are merged by their authors.
100+
101+
This is to ensure that PRs are merged as intended by the author,
102+
e.g. as a squash merge, by rebasing from the web interface or manually from the command line.
103+
104+
If you have multiple changes in one PR, do a rebase merge.
105+
Otherwise, you should usually do a squash merge.
106+
107+
If PR author does not have write access to the repository,
108+
maintainers who reviewed the PR can merge it.
109+
110+
If you do not have access to the repository and created a PR from a fork,
111+
ask the maintainers to merge the PR and say how it should be merged.
183112

184113
## Other ways to contribute
185114

186115
For other ways to contribute, refer to the [website](https://delta.chat/en/contribute).
116+
117+
You can find the list of good first issues
118+
and a link to this guide
119+
on the contributing page: <https://github.com/deltachat/deltachat-core-rust/contribute>
120+
121+
[Conventional Commits]: https://www.conventionalcommits.org/
122+
[git-cliff]: https://git-cliff.org/

STYLE.md

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
# Coding conventions
2+
3+
We format the code using `rustfmt`. Run `cargo fmt` prior to committing the code.
4+
Run `scripts/clippy.sh` to check the code for common mistakes with [Clippy].
5+
6+
[Clippy]: https://doc.rust-lang.org/clippy/
7+
8+
## SQL
9+
10+
Multi-line SQL statements should be formatted using string literals,
11+
for example
12+
```
13+
sql.execute(
14+
"CREATE TABLE messages (
15+
id INTEGER PRIMARY KEY AUTOINCREMENT,
16+
text TEXT DEFAULT '' NOT NULL -- message text
17+
) STRICT",
18+
)
19+
.await?;
20+
```
21+
22+
Do not use macros like [`concat!`](https://doc.rust-lang.org/std/macro.concat.html)
23+
or [`indoc!](https://docs.rs/indoc).
24+
Do not escape newlines like this:
25+
```
26+
sql.execute(
27+
"CREATE TABLE messages ( \
28+
id INTEGER PRIMARY KEY AUTOINCREMENT, \
29+
text TEXT DEFAULT '' NOT NULL \
30+
) STRICT",
31+
)
32+
.await?;
33+
```
34+
Escaping newlines
35+
is prone to errors like this if space before backslash is missing:
36+
```
37+
"SELECT foo\
38+
FROM bar"
39+
```
40+
Literal above results in `SELECT fooFROM bar` string.
41+
This style also does not allow using `--` comments.
42+
43+
---
44+
45+
Declare new SQL tables with [`STRICT`](https://sqlite.org/stricttables.html) keyword
46+
to make SQLite check column types.
47+
48+
Declare primary keys with [`AUTOINCREMENT`](https://www.sqlite.org/autoinc.html) keyword.
49+
This avoids reuse of the row IDs and can avoid dangerous bugs
50+
like forwarding wrong message because the message was deleted
51+
and another message took its row ID.
52+
53+
Declare all new columns as `NOT NULL`
54+
and set the `DEFAULT` value if it is optional so the column can be skipped in `INSERT` statements.
55+
Dealing with `NULL` values both in SQL and in Rust is tricky and we try to avoid it.
56+
If column is already declared without `NOT NULL`, use `IFNULL` function to provide default value when selecting it.
57+
Use `HAVING COUNT(*) > 0` clause
58+
to [prevent aggregate functions such as `MIN` and `MAX` from returning `NULL`](https://stackoverflow.com/questions/66527856/aggregate-functions-max-etc-return-null-instead-of-no-rows).
59+
60+
Don't delete unused columns too early, but maybe after several months/releases, unused columns are
61+
still used by older versions, so deleting them breaks downgrading the core or importing a backup in
62+
an older version. Also don't change the column type, consider adding a new column with another name
63+
instead. Finally, never change column semantics, this is especially dangerous because the `STRICT`
64+
keyword doesn't help here.
65+
66+
## Errors
67+
68+
Delta Chat core mostly uses [`anyhow`](https://docs.rs/anyhow/) errors.
69+
When using [`Context`](https://docs.rs/anyhow/latest/anyhow/trait.Context.html),
70+
capitalize it but do not add a full stop as the contexts will be separated by `:`.
71+
For example:
72+
```
73+
.with_context(|| format!("Unable to trash message {msg_id}"))
74+
```
75+
76+
All errors should be handled in one of these ways:
77+
- With `if let Err() =` (incl. logging them into `warn!()`/`err!()`).
78+
- With `.log_err().ok()`.
79+
- Bubbled up with `?`.
80+
81+
`backtrace` feature is enabled for `anyhow` crate
82+
and `debug = 1` option is set in the test profile.
83+
This allows to run `RUST_BACKTRACE=1 cargo test`
84+
and get a backtrace with line numbers in resultified tests
85+
which return `anyhow::Result`.
86+
87+
## Logging
88+
89+
For logging, use `info!`, `warn!` and `error!` macros.
90+
Log messages should be capitalized and have a full stop in the end. For example:
91+
```
92+
info!(context, "Ignoring addition of {added_addr:?} to {chat_id}.");
93+
```
94+
95+
Format anyhow errors with `{:#}` to print all the contexts like this:
96+
```
97+
error!(context, "Failed to set selfavatar timestamp: {err:#}.");
98+
```

0 commit comments

Comments
 (0)