diff options
| -rw-r--r-- | .github/PULL_REQUEST_TEMPLATE.md | 3 | ||||
| -rw-r--r-- | docs/_summary.md | 1 | ||||
| -rw-r--r-- | docs/pr_checklist.md | 125 |
3 files changed, 128 insertions, 1 deletions
diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index cbc018ea0..d402488d4 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md | |||
| @@ -26,7 +26,8 @@ | |||
| 26 | 26 | ||
| 27 | <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> | 27 | <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> |
| 28 | <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> | 28 | <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> |
| 29 | - [ ] My code follows the code style of this project. | 29 | - [ ] 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) |
| 30 | - [ ] I have read the [**PR Checklist** document](https://docs.qmk.fm/#/pr_checklist) and have made the appropriate changes. | ||
| 30 | - [ ] My change requires a change to the documentation. | 31 | - [ ] My change requires a change to the documentation. |
| 31 | - [ ] I have updated the documentation accordingly. | 32 | - [ ] I have updated the documentation accordingly. |
| 32 | - [ ] I have read the [**CONTRIBUTING** document](https://docs.qmk.fm/#/contributing). | 33 | - [ ] I have read the [**CONTRIBUTING** document](https://docs.qmk.fm/#/contributing). |
diff --git a/docs/_summary.md b/docs/_summary.md index 63c54ec21..9ed55a3d0 100644 --- a/docs/_summary.md +++ b/docs/_summary.md | |||
| @@ -111,6 +111,7 @@ | |||
| 111 | * [Velocikey](feature_velocikey.md) | 111 | * [Velocikey](feature_velocikey.md) |
| 112 | 112 | ||
| 113 | * Developing QMK | 113 | * Developing QMK |
| 114 | * [PR Checklist](pr_checklist.md) | ||
| 114 | * Breaking Changes | 115 | * Breaking Changes |
| 115 | * [Overview](breaking_changes.md) | 116 | * [Overview](breaking_changes.md) |
| 116 | * [My Pull Request Was Flagged](breaking_changes_instructions.md) | 117 | * [My Pull Request Was Flagged](breaking_changes_instructions.md) |
diff --git a/docs/pr_checklist.md b/docs/pr_checklist.md new file mode 100644 index 000000000..8755552b9 --- /dev/null +++ b/docs/pr_checklist.md | |||
| @@ -0,0 +1,125 @@ | |||
| 1 | # PR checklists | ||
| 2 | |||
| 3 | This is a non-exhaustive checklist of what the QMK collaborators will be checking when reviewing submitted PRs. | ||
| 4 | |||
| 5 | 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. | ||
| 6 | |||
| 7 | ## General PRs | ||
| 8 | |||
| 9 | - PR should be submitted using a non-`master` branch on the source repository | ||
| 10 | - This does not mean you target a different branch for your PR, rather that you're not working out of your own master branch | ||
| 11 | - 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) | ||
| 12 | - Newly-added directories and filenames must be lowercase | ||
| 13 | - This rule may be relaxed if upstream sources originally had uppercase characters (e.g. ChibiOS, or imported files from other repositories etc.) | ||
| 14 | - If there is enough justification (i.e. consistency with existing core files etc.) this can be relaxed | ||
| 15 | - a board designer naming their keyboard with uppercase letters is not enough justification | ||
| 16 | - Valid license headers on all `*.c` and `*.h` source files | ||
| 17 | - GPL2/GPL3 recommended for consistency | ||
| 18 | - 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. | ||
| 19 | - QMK codebase "best practices" followed | ||
| 20 | - This is not an exhaustive list, and will likely get amended as time goes by | ||
| 21 | - `#pragma once` instead of `#ifndef` include guards in header files | ||
| 22 | - No "old-school" GPIO/I2C/SPI functions used -- must use QMK abstractions unless justifiable (and laziness is not valid justification) | ||
| 23 | - Timing abstractions should be followed too: | ||
| 24 | - `wait_ms()` instead of `_delay_ms()` (remove `#include <util/delay.h>` too) | ||
| 25 | - `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 | ||
| 26 | - If you think a new abstraction is useful, you're encouraged to: | ||
| 27 | - prototype it in your own keyboard until it's feature-complete | ||
| 28 | - discuss it with QMK Collaborators on Discord | ||
| 29 | - refactor it as a separate core change | ||
| 30 | - remove your specific copy in your board | ||
| 31 | |||
| 32 | ## Core PRs | ||
| 33 | |||
| 34 | - Must now target `develop` branch, which will subsequently be merged back to `master` on the breaking changes timeline | ||
| 35 | - Other notes TBD | ||
| 36 | - Core is a lot more subjective given the breadth of posted changes | ||
| 37 | |||
| 38 | ## Keyboard PRs | ||
| 39 | |||
| 40 | Closed PRs (for inspiration, previous sets of review comments will help you eliminate ping-pong of your own reviews): | ||
| 41 | https://github.com/qmk/qmk_firmware/pulls?q=is%3Apr+is%3Aclosed+label%3Akeyboard | ||
| 42 | |||
| 43 | - `info.json` | ||
| 44 | - valid URL | ||
| 45 | - valid maintainer | ||
| 46 | - displays correctly in Configurator (press Ctrl+Shift+I to preview local file, turn on fast input to verify ordering) | ||
| 47 | - `readme.md` | ||
| 48 | - standard template should be present | ||
| 49 | - flash command has `:flash` at end | ||
| 50 | - 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. | ||
| 51 | - `rules.mk` | ||
| 52 | - removed `MIDI_ENABLE`, `FAUXCLICKY_ENABLE` and `HD44780_ENABLE` | ||
| 53 | - modified `# Enable Bluetooth with the Adafruit EZ-Key HID` -> `# Enable Bluetooth` | ||
| 54 | - No `(-/+size)` comments related to enabling features | ||
| 55 | - Remove the list of alternate bootloaders if one has been specified | ||
| 56 | - 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) | ||
| 57 | - keyboard `config.h` | ||
| 58 | - don't repeat `MANUFACTURER` in the `PRODUCT` value | ||
| 59 | - no `#define DESCRIPTION` | ||
| 60 | - no Magic Key Options, MIDI Options or HD44780 configuration | ||
| 61 | - user preference configurable `#define`s need to be moved to keymap `config.h` | ||
| 62 | - "`DEBOUNCE`" instead of "`DEBOUNCING_DELAY`" | ||
| 63 | - bare minimum required code for a board to boot into QMK should be present | ||
| 64 | - initialisation code for the matrix and critical devices | ||
| 65 | - mirroring existing functionality of a commercial board (like custom keycodes and special animations etc.) should be handled through non-`default` keymaps | ||
| 66 | - `keyboard.c` | ||
| 67 | - empty `xxxx_xxxx_kb()` or other weak-defined default implemented functions removed | ||
| 68 | - commented-out functions removed too | ||
| 69 | - `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) | ||
| 70 | - prefer `CUSTOM_MATRIX = lite` if custom matrix used, allows for standard debounce, see [custom matrix 'lite'](https://docs.qmk.fm/#/custom_matrix?id=lite) | ||
| 71 | - `keyboard.h` | ||
| 72 | - `#include "quantum.h"` appears at the top | ||
| 73 | - `LAYOUT` macros should use standard definitions if applicable | ||
| 74 | - Use the Community Layout macro names where they apply (preferred above `LAYOUT`/`LAYOUT_all`) | ||
| 75 | - keymap `config.h` | ||
| 76 | - no duplication of `rules.mk` or `config.h` from keyboard | ||
| 77 | - `keymaps/default/keymap.c` | ||
| 78 | - `QMKBEST`/`QMKURL` removed (sheesh) | ||
| 79 | - 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... | ||
| 80 | ``` | ||
| 81 | layer_state_t layer_state_set_user(layer_state_t state) { | ||
| 82 | return update_tri_layer_state(state, _LOWER, _RAISE, _ADJUST); | ||
| 83 | } | ||
| 84 | ``` | ||
| 85 | ...instead of manually handling `layer_on()`, `update_tri_layer()` inside the keymap's `process_record_user()`. | ||
| 86 | - default (and via) keymaps should be "pristine" | ||
| 87 | - bare minimum to be used as a "clean slate" for another user to develop their own user-specific keymap | ||
| 88 | - standard layouts preferred in these keymaps, if possible | ||
| 89 | - 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 | ||
| 90 | - submitters can also have a "manufacturer-matching" keymap that mirrors existing functionality of the commercial product, if porting an existing board | ||
| 91 | |||
| 92 | Also, specific to ChibiOS: | ||
| 93 | - **Strong** preference to using existing ChibiOS board definitions. | ||
| 94 | - 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 | ||
| 95 | - Example: For an STM32L082KZ, given the similarity to an STM32L073RZ, you can use `BOARD = ST_NUCLEO64_L073RZ` in rules.mk | ||
| 96 | - QMK is migrating to not having custom board definitions if at all possible, due to the ongoing maintenance burden when upgrading ChibiOS | ||
| 97 | - If a board definition is unavoidable, `board.c` must have a standard `__early_init()` (as per normal ChibiOS board defs) and an empty `boardInit()`: | ||
| 98 | - see Arm/ChibiOS [early initialization](https://docs.qmk.fm/#/platformdev_chibios_earlyinit?id=board-init) | ||
| 99 | - `__early_init()` should be replaced by either `early_hardware_init_pre()` or `early_hardware_init_post()` as appropriate | ||
| 100 | - `boardInit()` should be migrated to `board_init()` | ||
| 101 | |||
| 102 | ## Keymap PRs | ||
| 103 | |||
| 104 | - `#include QMK_KEYBOARD_H` preferred to including specific board files | ||
| 105 | - Prefer layer `enum`s to `#define`s | ||
| 106 | - Require custom keycode `enum`s to `#define`s, first entry must have ` = SAFE_RANGE` | ||
| 107 | - Terminating backslash (`\`) in lines of LAYOUT macro parameters is superfluous | ||
| 108 | - Some care with spacing (e.g., alignment on commas or first char of keycodes) makes for a much nicer-looking keymap | ||
| 109 | |||
| 110 | --- | ||
| 111 | |||
| 112 | ## Notes | ||
| 113 | |||
| 114 | For when people use their own `master` branch, post this after merge: | ||
| 115 | ``` | ||
| 116 | 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. | ||
| 117 | |||
| 118 | There are instructions on how to keep your fork updated here: | ||
| 119 | |||
| 120 | [**Best Practices: Your Fork's Master: Update Often, Commit Never**](https://docs.qmk.fm/#/newbs_git_using_your_master_branch) | ||
| 121 | |||
| 122 | [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. | ||
| 123 | |||
| 124 | Thanks for contributing! | ||
| 125 | ``` | ||
