Add PR checklist document. (#9913)
* Add PR checklist document. * Update docs/pr_checklist.md Co-authored-by: James Young <18669334+noroadsleft@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Ryan <fauxpark@gmail.com> * Reword the lower/raise/adjust suggestion somewhat for clarity. * Add suggestion from @Didel for coding conventions. Co-authored-by: James Young <18669334+noroadsleft@users.noreply.github.com> Co-authored-by: Ryan <fauxpark@gmail.com>
This commit is contained in:
Nick Brassel
GitHub
3
.github/PULL_REQUEST_TEMPLATE.md
vendored
3
.github/PULL_REQUEST_TEMPLATE.md
vendored
@@ -26,7 +26,8 @@
|
||||
|
||||
<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
|
||||
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
|
||||
- [ ] My code follows the code style of this project.
|
||||
- [ ] My code follows the code style of this project: [**C**](https://docs.qmk.fm/#/coding_conventions_c), [**Python**](https://docs.qmk.fm/#/coding_conventions_python)
|
||||
- [ ] I have read the [**PR Checklist** document](https://docs.qmk.fm/#/pr_checklist) and have made the appropriate changes.
|
||||
- [ ] My change requires a change to the documentation.
|
||||
- [ ] I have updated the documentation accordingly.
|
||||
- [ ] I have read the [**CONTRIBUTING** document](https://docs.qmk.fm/#/contributing).
|
||||
|
||||
@@ -111,6 +111,7 @@
|
||||
* [Velocikey](feature_velocikey.md)
|
||||
|
||||
* Developing QMK
|
||||
* [PR Checklist](pr_checklist.md)
|
||||
* Breaking Changes
|
||||
* [Overview](breaking_changes.md)
|
||||
* [My Pull Request Was Flagged](breaking_changes_instructions.md)
|
||||
|
||||
125
docs/pr_checklist.md
Normal file
125
docs/pr_checklist.md
Normal file
@@ -0,0 +1,125 @@
|
||||
# PR checklists
|
||||
|
||||
This is a non-exhaustive checklist of what the QMK collaborators will be checking when reviewing submitted PRs.
|
||||
|
||||
If there are any inconsistencies with these recommendations, you're best off [creating an issue](https://github.com/qmk/qmk_firmware/issues/new) against this document, or getting in touch with a QMK Collaborator on Discord.
|
||||
|
||||
## General PRs
|
||||
|
||||
- PR should be submitted using a non-`master` branch on the source repository
|
||||
- This does not mean you target a different branch for your PR, rather that you're not working out of your own master branch
|
||||
- If submitter _does_ use their own `master` branch, they'll be given a link to the ["how to git"](https://docs.qmk.fm/#/newbs_git_using_your_master_branch) page after merging -- (end of this document will contain the contents of the message)
|
||||
- Newly-added directories and filenames must be lowercase
|
||||
- This rule may be relaxed if upstream sources originally had uppercase characters (e.g. ChibiOS, or imported files from other repositories etc.)
|
||||
- If there is enough justification (i.e. consistency with existing core files etc.) this can be relaxed
|
||||
- a board designer naming their keyboard with uppercase letters is not enough justification
|
||||
- Valid license headers on all `*.c` and `*.h` source files
|
||||
- GPL2/GPL3 recommended for consistency
|
||||
- Other licenses are permitted, however they must be GPL-compatible and must allow for redistribution. Using a different license will almost certainly delay a PR getting merged.
|
||||
- QMK codebase "best practices" followed
|
||||
- This is not an exhaustive list, and will likely get amended as time goes by
|
||||
- `#pragma once` instead of `#ifndef` include guards in header files
|
||||
- No "old-school" GPIO/I2C/SPI functions used -- must use QMK abstractions unless justifiable (and laziness is not valid justification)
|
||||
- Timing abstractions should be followed too:
|
||||
- `wait_ms()` instead of `_delay_ms()` (remove `#include <util/delay.h>` too)
|
||||
- `timer_read()` and `timer_read32()` etc. -- see [timer.h](https://github.com/qmk/qmk_firmware/blob/master/tmk_core/common/timer.h) for the timing APIs
|
||||
- If you think a new abstraction is useful, you're encouraged to:
|
||||
- prototype it in your own keyboard until it's feature-complete
|
||||
- discuss it with QMK Collaborators on Discord
|
||||
- refactor it as a separate core change
|
||||
- remove your specific copy in your board
|
||||
|
||||
## Core PRs
|
||||
|
||||
- Must now target `develop` branch, which will subsequently be merged back to `master` on the breaking changes timeline
|
||||
- Other notes TBD
|
||||
- Core is a lot more subjective given the breadth of posted changes
|
||||
|
||||
## Keyboard PRs
|
||||
|
||||
Closed PRs (for inspiration, previous sets of review comments will help you eliminate ping-pong of your own reviews):
|
||||
https://github.com/qmk/qmk_firmware/pulls?q=is%3Apr+is%3Aclosed+label%3Akeyboard
|
||||
|
||||
- `info.json`
|
||||
- valid URL
|
||||
- valid maintainer
|
||||
- displays correctly in Configurator (press Ctrl+Shift+I to preview local file, turn on fast input to verify ordering)
|
||||
- `readme.md`
|
||||
- standard template should be present
|
||||
- flash command has `:flash` at end
|
||||
- valid hardware availability link (unless handwired) -- private groupbuys are okay, but one-off prototypes will be questioned. If open-source, a link to files should be provided.
|
||||
- `rules.mk`
|
||||
- removed `MIDI_ENABLE`, `FAUXCLICKY_ENABLE` and `HD44780_ENABLE`
|
||||
- modified `# Enable Bluetooth with the Adafruit EZ-Key HID` -> `# Enable Bluetooth`
|
||||
- No `(-/+size)` comments related to enabling features
|
||||
- Remove the list of alternate bootloaders if one has been specified
|
||||
- No re-definitions of the default MCU parameters if same value, when compared to the equivalent MCU in [mcu_selection.mk](https://github.com/qmk/qmk_firmware/blob/master/quantum/mcu_selection.mk)
|
||||
- keyboard `config.h`
|
||||
- don't repeat `MANUFACTURER` in the `PRODUCT` value
|
||||
- no `#define DESCRIPTION`
|
||||
- no Magic Key Options, MIDI Options or HD44780 configuration
|
||||
- user preference configurable `#define`s need to be moved to keymap `config.h`
|
||||
- "`DEBOUNCE`" instead of "`DEBOUNCING_DELAY`"
|
||||
- bare minimum required code for a board to boot into QMK should be present
|
||||
- initialisation code for the matrix and critical devices
|
||||
- mirroring existing functionality of a commercial board (like custom keycodes and special animations etc.) should be handled through non-`default` keymaps
|
||||
- `keyboard.c`
|
||||
- empty `xxxx_xxxx_kb()` or other weak-defined default implemented functions removed
|
||||
- commented-out functions removed too
|
||||
- `matrix_init_board()` etc. migrated to `keyboard_pre_init_kb()`, see: [keyboard_pre_init*](https://docs.qmk.fm/#/custom_quantum_functions?id=keyboard_pre_init_-function-documentation)
|
||||
- prefer `CUSTOM_MATRIX = lite` if custom matrix used, allows for standard debounce, see [custom matrix 'lite'](https://docs.qmk.fm/#/custom_matrix?id=lite)
|
||||
- `keyboard.h`
|
||||
- `#include "quantum.h"` appears at the top
|
||||
- `LAYOUT` macros should use standard definitions if applicable
|
||||
- Use the Community Layout macro names where they apply (preferred above `LAYOUT`/`LAYOUT_all`)
|
||||
- keymap `config.h`
|
||||
- no duplication of `rules.mk` or `config.h` from keyboard
|
||||
- `keymaps/default/keymap.c`
|
||||
- `QMKBEST`/`QMKURL` removed (sheesh)
|
||||
- If using `MO(_LOWER)` and `MO(_RAISE)` keycodes or equivalent, and the keymap has an adjust layer when holding both keys -- if the keymap has no "direct-to-adjust" keycode (such as `MO(_ADJUST)`) then you should prefer to write...
|
||||
```
|
||||
layer_state_t layer_state_set_user(layer_state_t state) {
|
||||
return update_tri_layer_state(state, _LOWER, _RAISE, _ADJUST);
|
||||
}
|
||||
```
|
||||
...instead of manually handling `layer_on()`, `update_tri_layer()` inside the keymap's `process_record_user()`.
|
||||
- default (and via) keymaps should be "pristine"
|
||||
- bare minimum to be used as a "clean slate" for another user to develop their own user-specific keymap
|
||||
- standard layouts preferred in these keymaps, if possible
|
||||
- submitters can have a personal (or bells-and-whistles) keymap showcasing capabilities in the same PR but it shouldn't be embedded in the 'default' keymap
|
||||
- submitters can also have a "manufacturer-matching" keymap that mirrors existing functionality of the commercial product, if porting an existing board
|
||||
|
||||
Also, specific to ChibiOS:
|
||||
- **Strong** preference to using existing ChibiOS board definitions.
|
||||
- A lot of the time, an equivalent Nucleo board can be used with a different flash size or slightly different model in the same family
|
||||
- Example: For an STM32L082KZ, given the similarity to an STM32L073RZ, you can use `BOARD = ST_NUCLEO64_L073RZ` in rules.mk
|
||||
- QMK is migrating to not having custom board definitions if at all possible, due to the ongoing maintenance burden when upgrading ChibiOS
|
||||
- If a board definition is unavoidable, `board.c` must have a standard `__early_init()` (as per normal ChibiOS board defs) and an empty `boardInit()`:
|
||||
- see Arm/ChibiOS [early initialization](https://docs.qmk.fm/#/platformdev_chibios_earlyinit?id=board-init)
|
||||
- `__early_init()` should be replaced by either `early_hardware_init_pre()` or `early_hardware_init_post()` as appropriate
|
||||
- `boardInit()` should be migrated to `board_init()`
|
||||
|
||||
## Keymap PRs
|
||||
|
||||
- `#include QMK_KEYBOARD_H` preferred to including specific board files
|
||||
- Prefer layer `enum`s to `#define`s
|
||||
- Require custom keycode `enum`s to `#define`s, first entry must have ` = SAFE_RANGE`
|
||||
- Terminating backslash (`\`) in lines of LAYOUT macro parameters is superfluous
|
||||
- Some care with spacing (e.g., alignment on commas or first char of keycodes) makes for a much nicer-looking keymap
|
||||
|
||||
---
|
||||
|
||||
## Notes
|
||||
|
||||
For when people use their own `master` branch, post this after merge:
|
||||
```
|
||||
For future reference, we recommend against committing to your `master` branch as you've done here, because pull requests from modified `master` branches can make it more difficult to keep your QMK fork updated. It is highly recommended for QMK development – regardless of what is being done or where – to keep your master updated, but **NEVER** commit to it. Instead, do all your changes in a branch (branches are basically free in Git) and issue PRs from your branches when you're developing.
|
||||
|
||||
There are instructions on how to keep your fork updated here:
|
||||
|
||||
[**Best Practices: Your Fork's Master: Update Often, Commit Never**](https://docs.qmk.fm/#/newbs_git_using_your_master_branch)
|
||||
|
||||
[Fixing Your Branch](https://docs.qmk.fm/#/newbs_git_resynchronize_a_branch) will walk you through fixing up your `master` branch moving forward. If you need any help with this just ask.
|
||||
|
||||
Thanks for contributing!
|
||||
```
|
||||
Reference in New Issue
Block a user