From bafa15c8f1d576c81024494ed6e175cf848fdd34 Mon Sep 17 00:00:00 2001 From: Gideon <87426140+GideonBear@users.noreply.github.com> Date: Sun, 2 Nov 2025 16:40:11 +0100 Subject: [PATCH] docs: Improve CONTRIBUTING.md (#1420) --- CONTRIBUTING.md | 105 +++++++++++++++++++++++------------------------- 1 file changed, 50 insertions(+), 55 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1f9a6a02..df06cfe7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -7,13 +7,24 @@ We welcome and encourage contributions of all kinds, such as: 2. Documentation improvements 3. Code (PR or PR Review) -Please follow the [Karma Runner guidelines](http://karma-runner.github.io/6.2/dev/git-commit-msg.html) -for commit messages. +### General guidelines -## Adding a new `step` +**Please use [conventional commits](https://www.conventionalcommits.org/en/v1.0.0/) for your PR title**. -In `topgrade`'s term, package manager is called `step`. -To add a new `step` to `topgrade`: +We use [pre-commit](https://github.com/pre-commit/pre-commit). It runs in CI, but you can optionally install the hook +locally with `pre-commit install`. If you don't want to use pre-commit, make sure the following pass before submitting +your PR: + +```shell +$ cargo fmt +$ cargo clippy +$ cargo test +``` + +### Adding a new step + +In `topgrade`'s terms, a package manager (or something else that can be upgraded) is called a step. +To add a new step to `topgrade`: 1. Add a new variant to [`enum Step`](https://github.com/topgrade-rs/topgrade/blob/main/src/step.rs) @@ -33,8 +44,9 @@ To add a new `step` to `topgrade`: You need to find the appropriate location where this update function goes, it should be a file under [`src/steps`](https://github.com/topgrade-rs/topgrade/tree/main/src/steps), - the file names are self-explanatory, for example, `step`s related to `zsh` are - placed in [`steps/zsh.rs`](https://github.com/topgrade-rs/topgrade/blob/main/src/steps/zsh.rs). + the file names are self-explanatory, for example, steps related to `zsh` are + placed in [`steps/zsh.rs`](https://github.com/topgrade-rs/topgrade/blob/main/src/steps/zsh.rs), and steps that run on + Linux only are placed in [`steps/linux.rs`](https://github.com/topgrade-rs/topgrade/blob/main/src/steps/linux.rs). Then you implement the update function, and put it in the file where it belongs. @@ -53,19 +65,17 @@ To add a new `step` to `topgrade`: } ``` - Such a update function would be conventionally named `run_xxx()`, where `xxx` - is the name of the new step, and it should take a argument of type - `&ExecutionContext`, this is adequate for most cases unless some extra stuff is - needed (You can find some examples where extra arguments are needed - [here](https://github.com/topgrade-rs/topgrade/blob/7e48c5dedcfd5d0124bb9f39079a03e27ed23886/src/main.rs#L201-L219)). + Such an update function would be conventionally named `run_xxx()`, where `xxx` + is the name of the new step, and it should take an argument of type + `&ExecutionContext`. - Update function would usually do 3 things: + The update function should usually do 3 things: 1. Check if the step is installed - 2. Output the Separator - 3. Invoke the step + 2. Output the separator + 3. Execute commands - Still, this is sufficient for most tools, but you may need some extra stuff - with complicated `step`. + This is sufficient for most tools, but you may need some extra stuff + for complicated steps. 3. Add a match arm to `Step::run()` @@ -74,7 +84,7 @@ To add a new `step` to `topgrade`: ``` We use [conditional compilation](https://doc.rust-lang.org/reference/conditional-compilation.html) - to separate the steps, for example, for steps that are Linux-only, it goes + to separate the steps. For example, for steps that are Linux-only, it goes like this: ```rust @@ -89,11 +99,11 @@ To add a new `step` to `topgrade`: ```rust steps.push(Xxx) ``` - Try to keep the conditional compilation the same as in the above step 3. + Keep the conditional compilation the same as in the above step 3. - Congrats, you just added a new `step` :) + Congrats, you just added a new step :) -## Modification to the configuration entries +### Modification to the configuration entries If your PR has the configuration options (in [`src/config.rs`](https://github.com/topgrade-rs/topgrade/blob/main/src/config.rs)) @@ -106,10 +116,10 @@ Be sure to apply your changes to [`config.example.toml`](https://github.com/topgrade-rs/topgrade/blob/main/config.example.toml), and have some basic documentations guiding user how to use these options. -## Breaking changes +### Breaking changes -If your PR introduces a breaking change, document it in [`BREAKINGCHANGES_dev.md`][bc_dev], -it should be written in Markdown and wrapped at 80, for example: +If your PR introduces a breaking change, document it in [`BREAKINGCHANGES_dev.md`][bc_dev]. +It should be written in Markdown and wrapped at 80, for example: ```md 1. The configuration location has been updated to x. @@ -121,25 +131,12 @@ it should be written in Markdown and wrapped at 80, for example: [bc_dev]: https://github.com/topgrade-rs/topgrade/blob/main/BREAKINGCHANGES_dev.md -## Before you submit your PR - -Make sure your patch passes the following tests on your host: - -```shell -$ cargo build -$ cargo fmt -$ cargo clippy -$ cargo test -``` - -Don't worry about other platforms, we have most of them covered in our CI. - -## I18n +### I18n If your PR introduces user-facing messages, we need to ensure they are translated. Please add the translations to [`locales/app.yml`][app_yml]. For simple messages without arguments (e.g., "hello world"), we can simply translate them according -(Tip: ChatGPT or similar LLMs is good at translation). If a message contains +(Tip: LLMs are good at translation). If a message contains arguments, e.g., "hello ", please follow this convention: ```yml @@ -152,24 +149,22 @@ a preceding `%` when used in translations. [app_yml]: https://github.com/topgrade-rs/topgrade/blob/main/locales/app.yml -## Some tips +### Locales -1. Locale +Some steps respect locale, which means their output can be in language other +than English. In those cases, we cannot rely on the output of a command. - Some `step` respects locale, which means their output can be in language other - than English, we should not do check on it. +For example, one may want to check if a tool works by doing this: - For example, one may want to check if a tool works by doing this: +```rust +let output = Command::new("xxx").arg("--help").output().unwrap(); +let stdout = from_utf8(output.stdout).expect("Assume it is UTF-8 encoded"); - ```rust - let output = Command::new("xxx").arg("--help").output().unwrap(); - let stdout = from_utf8(output.stdout).expect("Assume it is UTF-8 encoded"); +if stdout.contains("help") { +// xxx works +} +``` - if stdout.contains("help") { - // xxx works - } - ``` - - If `xxx` respects locale, then the above code should work on English system, - on a system that does not use English, e.g., it uses Chinese, that `"help"` may be - translated to `"帮助"`, and the above code won't work. +If `xxx` respects locale, then the above code should work on English system, +on a system that does not use English, e.g., it uses Chinese, that `"help"` may be +translated to `"帮助"`, and the above code won't work.