Skip to content

Commit cc8792a

Browse files
committed
docs(code review): add a section explaining what to do on a code review
1 parent c5e7767 commit cc8792a

3 files changed

Lines changed: 77 additions & 12 deletions

File tree

COMPLIANCE.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,8 @@ ArduPilot Methodic Configurator adheres to multiple compliance standards and bes
99
- Relevant documentation opens automatically in a browser window
1010
- Uses *What you see is what gets changed* paradigm. No parameters are changed without the users's knowledge
1111
- Translated into multiple languages
12-
- No visible menus, no hidden menus.
12+
- No visible menus, no hidden menus
13+
- Keyboard-only navigation possible
1314

1415
## Coding Standards
1516

CONTRIBUTING.md

Lines changed: 74 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -102,24 +102,88 @@ git rebase --signoff master
102102

103103
Once your changes are ready, submit a [GitHub Pull Request (PR)](https://github.com/ArduPilot/MethodicConfigurator/pulls).
104104

105+
## Code review process
106+
107+
Once your pull request is submitted, a thorough code review process will begin.
108+
We evaluate contributions based on multiple criteria to ensure quality, security, and maintainability.
109+
The review includes both automated checks and manual inspection.
110+
111+
### Review Process
112+
113+
1. **Initial Automated Review**: CI checks must pass before manual review begins
114+
2. **Peer Review**: At least one maintainer reviews the code, adding comments and suggestion to the github pull request webpage
115+
3. **Feedback & Iteration**: Contributors address review comments and update the PR
116+
4. **Final Approval**: Maintainers approve and merge the changes
117+
5. **Post-Merge**: Automated deployment and monitoring ensure stability
118+
119+
### Automated Checks
120+
121+
* **CI/CD Workflows**: All changes are automatically tested against our
122+
[code quality guidelines](https://ardupilot.github.io/MethodicConfigurator/COMPLIANCE.html#coding-standards)
123+
and [security requirements](https://ardupilot.github.io/MethodicConfigurator/SECURITY)
124+
* **Linting and Formatting**: Code must pass [Ruff](https://github.com/ArduPilot/MethodicConfigurator/actions/workflows/ruff.yml),
125+
[Pylint](https://github.com/ArduPilot/MethodicConfigurator/actions/workflows/pylint.yml), and other quality checks
126+
* **Type Checking**: [MyPy](https://github.com/ArduPilot/MethodicConfigurator/actions/workflows/mypy.yml) and
127+
[Pyright](https://github.com/ArduPilot/MethodicConfigurator/actions/workflows/pyright.yml) validation
128+
* **Security Scanning**: Automated vulnerability detection via [CodeQL](https://github.com/ArduPilot/MethodicConfigurator/actions/workflows/codeql.yml) and dependency reviews
129+
* **Testing**: Comprehensive test suite execution with [pytest](https://github.com/ArduPilot/MethodicConfigurator/actions/workflows/pytest.yml)
130+
131+
### Manual Review Criteria
132+
133+
We manually verify the following aspects:
134+
135+
#### Code Quality & Standards
136+
137+
* Does the code follow our [coding standards](https://ardupilot.github.io/MethodicConfigurator/COMPLIANCE#coding-standards)
138+
and [PEP 8](https://peps.python.org/pep-0008/) guidelines?
139+
* Is the code well-documented with appropriate docstrings and comments?
140+
* Does it include comprehensive error handling and logging?
141+
* Are there any code smells, technical debt, or maintainability issues?
142+
* Do the [git commit messages follow conventional commit standards](https://www.conventionalcommits.org/en/v1.0.0/)?
143+
* Is at least the last commit in the pull request branch [signed off](https://developercertificate.org/) by the contributor?
144+
* Does the pull request have a clear description of the changes and their rationale?
145+
* Is the pull request branch free of merge commits?
146+
147+
#### Architecture & Design
148+
149+
* Does the change follow our [architecture guidelines](https://github.com/ArduPilot/MethodicConfigurator/blob/master/ARCHITECTURE.md)?
150+
* Is there proper separation of concerns (backend, business logic, frontend/GUI)?
151+
* Is the code modular, testable, and maintainable?
152+
* Does it follow object-oriented design principles and [clean code practices](https://www.oreilly.com/library/view/clean-code/9780136083238/)?
153+
154+
#### Functionality & Testing
155+
156+
* Is it generic enough to be useful for multiple use cases?
157+
* Does the change include appropriate unit tests and integration tests?
158+
* Are edge cases and error conditions properly tested?
159+
* Does it maintain or improve [test coverage](https://coveralls.io/github/ArduPilot/MethodicConfigurator)?
160+
* Have manual testing scenarios been considered?
161+
162+
#### Security & Compliance
163+
164+
* Does the change follow [secure coding practices](https://ardupilot.github.io/MethodicConfigurator/SECURITY)?
165+
* Does it comply with our [license requirements](https://github.com/ArduPilot/MethodicConfigurator/blob/master/LICENSE.md) and [REUSE specification](https://reuse.software/spec-3.3/)?
166+
167+
#### User Experience & Documentation
168+
169+
* Does the change maintain or improve usability?
170+
* Is user-facing documentation updated (including translations)?
171+
* Are there any breaking changes that need documentation?
172+
* New strings must be properly internationalized with `_( ... )`
173+
174+
#### Community & Process
175+
176+
* Does the change violate our [code of conduct](https://github.com/ArduPilot/MethodicConfigurator/blob/master/CODE_OF_CONDUCT.md)?
177+
105178
## Development Team
106179

107180
The ArduPilot Methodic Configurator project is open-source and maintained by a team of volunteers.
108181

109-
New developers are recommended to join the `#general` channel on
182+
New developers are recommended to join the `#general` and `#methodic_configurator` channels on
110183
[Discord](https://ardupilot.org/discord).
111184

112185
You can also join the
113186
[development discussion on Discourse](https://discuss.ardupilot.org/c/development-team).
114187

115188
Note that these are NOT for user tech support, and are moderated
116189
for new users to prevent off-topic discussion.
117-
118-
<!-- Gurubase Widget -->
119-
<script async src="https://widget.gurubase.io/widget.latest.min.js"
120-
data-widget-id="uE4kxEE4LY3ZSyfNsF5bU6gIOnWGTBOL_e16KwDH-0g"
121-
data-text="Ask AI"
122-
data-margins='{"bottom": "1rem", "right": "1rem"}'
123-
data-light-mode="true"
124-
id="guru-widget-id">
125-
</script>

SYSTEM_REQUIREMENTS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ To semi-automate the steps and processes on that guide the following *system des
128128
- [Test-driven development](https://en.wikipedia.org/wiki/Test-driven_development) (TDD)
129129
- [DevOps](https://en.wikipedia.org/wiki/DevOps)
130130
- [CI/CD automation](https://en.wikipedia.org/wiki/CI/CD)
131-
- git pre-commit hooks for code linting and other code quality checks
131+
- [git pre-commit hooks](https://github.com/ArduPilot/MethodicConfigurator/blob/master/.pre-commit-config.yaml) for code linting and other code quality checks
132132
- create command-line autocompletion for bash, zsh and powershell [PR #134](https://github.com/ArduPilot/MethodicConfigurator/pull/134)
133133

134134
## 9. Vehicle components and connections

0 commit comments

Comments
 (0)