More PR checklist updates (#14705)
* Wording, clarification. * Apply suggestions from code review Co-authored-by: Mikkel Jeppesen <2756925+Duckle29@users.noreply.github.com> Co-authored-by: Mikkel Jeppesen <2756925+Duckle29@users.noreply.github.com>
This commit is contained in:
parent
7a49e5d207
commit
4676a14596
1 changed files with 21 additions and 13 deletions
|
@ -4,14 +4,14 @@ This is a non-exhaustive checklist of what the QMK Collaborators will be checkin
|
||||||
|
|
||||||
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](https://discord.gg/Uq7gcHh).
|
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](https://discord.gg/Uq7gcHh).
|
||||||
|
|
||||||
## General PRs
|
## Requirements for all PRs
|
||||||
|
|
||||||
- PR should be submitted using a non-`master` branch on the source repository
|
- 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
|
- 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)
|
- 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
|
- 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.)
|
- this rule may be relaxed if upstream sources originally had uppercase characters (e.g. LUFA, 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
|
- if there is valid 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
|
- a board designer naming their keyboard with uppercase letters is not enough justification
|
||||||
- valid license headers on all `*.c` and `*.h` source files
|
- valid license headers on all `*.c` and `*.h` source files
|
||||||
- GPL2/GPL3 recommended for consistency
|
- GPL2/GPL3 recommended for consistency
|
||||||
|
@ -21,7 +21,7 @@ If there are any inconsistencies with these recommendations, you're best off [cr
|
||||||
- QMK Codebase "best practices" followed
|
- QMK Codebase "best practices" followed
|
||||||
- this is not an exhaustive list, and will likely get amended as time goes by
|
- 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
|
- `#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)
|
- no "old-school" or other low-level GPIO/I2C/SPI functions may be used -- must use QMK abstractions unless justifiable (and laziness is not valid justification)
|
||||||
- timing abstractions should be followed too:
|
- timing abstractions should be followed too:
|
||||||
- `wait_ms()` instead of `_delay_ms()` (remove `#include <util/delay.h>` 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
|
- `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
|
||||||
|
@ -30,7 +30,7 @@ If there are any inconsistencies with these recommendations, you're best off [cr
|
||||||
- discuss it with QMK Collaborators on Discord
|
- discuss it with QMK Collaborators on Discord
|
||||||
- refactor it as a separate core change
|
- refactor it as a separate core change
|
||||||
- remove your specific copy in your board
|
- remove your specific copy in your board
|
||||||
- rebase and fix all merge conflicts before opening the PR (in case you need help or advice, reach out to QMK Collaborators on Discord)
|
- fix all merge conflicts before opening the PR (in case you need help or advice, reach out to QMK Collaborators on Discord)
|
||||||
|
|
||||||
## Keymap PRs
|
## Keymap PRs
|
||||||
|
|
||||||
|
@ -50,11 +50,13 @@ https://github.com/qmk/qmk_firmware/pulls?q=is%3Apr+is%3Aclosed+label%3Akeyboard
|
||||||
- valid maintainer
|
- valid maintainer
|
||||||
- displays correctly in Configurator (press Ctrl+Shift+I to preview local file, turn on fast input to verify ordering)
|
- displays correctly in Configurator (press Ctrl+Shift+I to preview local file, turn on fast input to verify ordering)
|
||||||
- `readme.md`
|
- `readme.md`
|
||||||
- standard template should be present
|
- standard template should be present -- [link to template](https://github.com/qmk/qmk_firmware/blob/master/data/templates/avr/readme.md)
|
||||||
- flash command has `:flash` at end
|
- flash command is present, and 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.
|
- 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.
|
||||||
- clear instructions on how to reset the board into bootloader mode
|
- clear instructions on how to reset the board into bootloader mode
|
||||||
- a picture about the keyboard and preferably about the PCB, too
|
- a picture about the keyboard and preferably about the PCB, too
|
||||||
|
- images are not to be placed in the `qmk_firmware` repository
|
||||||
|
- images should be uploaded to an external image hosting service, such as [imgur](https://imgur.com/).
|
||||||
- `rules.mk`
|
- `rules.mk`
|
||||||
- removed `MIDI_ENABLE`, `FAUXCLICKY_ENABLE` and `HD44780_ENABLE`
|
- removed `MIDI_ENABLE`, `FAUXCLICKY_ENABLE` and `HD44780_ENABLE`
|
||||||
- modified `# Enable Bluetooth with the Adafruit EZ-Key HID` -> `# Enable Bluetooth`
|
- modified `# Enable Bluetooth with the Adafruit EZ-Key HID` -> `# Enable Bluetooth`
|
||||||
|
@ -71,20 +73,20 @@ https://github.com/qmk/qmk_firmware/pulls?q=is%3Apr+is%3Aclosed+label%3Akeyboard
|
||||||
- initialisation code for the matrix and critical devices
|
- 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
|
- mirroring existing functionality of a commercial board (like custom keycodes and special animations etc.) should be handled through non-`default` keymaps
|
||||||
- Vial-related files or changes will not be accepted, as they are not used by QMK firmware (no Vial-specific core code has been submitted or merged)
|
- Vial-related files or changes will not be accepted, as they are not used by QMK firmware (no Vial-specific core code has been submitted or merged)
|
||||||
- `keyboard.c`
|
- `<keyboard>.c`
|
||||||
- empty `xxxx_xxxx_kb()` or other weak-defined default implemented functions removed
|
- empty `xxxx_xxxx_kb()` or other weak-defined default implemented functions removed
|
||||||
- commented-out functions removed too
|
- 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)
|
- `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)
|
- prefer `CUSTOM_MATRIX = lite` if custom matrix used, allows for standard debounce, see [custom matrix 'lite'](https://docs.qmk.fm/#/custom_matrix?id=lite)
|
||||||
- prefer LED indicator [Configuration Options](https://docs.qmk.fm/#/feature_led_indicators?id=configuration-options) to custom `led_update_*()` implementations where possible
|
- prefer LED indicator [Configuration Options](https://docs.qmk.fm/#/feature_led_indicators?id=configuration-options) to custom `led_update_*()` implementations where possible
|
||||||
- `keyboard.h`
|
- `<keyboard>.h`
|
||||||
- `#include "quantum.h"` appears at the top
|
- `#include "quantum.h"` appears at the top
|
||||||
- `LAYOUT` macros should use standard definitions if applicable
|
- `LAYOUT` macros should use standard definitions if applicable
|
||||||
- use the Community Layout macro names where they apply (preferred above `LAYOUT`/`LAYOUT_all`)
|
- use the Community Layout macro names where they apply (preferred above `LAYOUT`/`LAYOUT_all`)
|
||||||
- keymap `config.h`
|
- keymap `config.h`
|
||||||
- no duplication of `rules.mk` or `config.h` from keyboard
|
- no duplication of `rules.mk` or `config.h` from keyboard
|
||||||
- `keymaps/default/keymap.c`
|
- `keymaps/default/keymap.c`
|
||||||
- `QMKBEST`/`QMKURL` removed (sheesh)
|
- `QMKBEST`/`QMKURL` removed
|
||||||
- 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...
|
- 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) {
|
layer_state_t layer_state_set_user(layer_state_t state) {
|
||||||
|
@ -100,7 +102,6 @@ https://github.com/qmk/qmk_firmware/pulls?q=is%3Apr+is%3Aclosed+label%3Akeyboard
|
||||||
- submitters can also have a "manufacturer-matching" keymap that mirrors existing functionality of the commercial product, if porting an existing board
|
- submitters can also have a "manufacturer-matching" keymap that mirrors existing functionality of the commercial product, if porting an existing board
|
||||||
- Do not include VIA json files in the PR. These do not belong in the QMK repository as they are not used by QMK firmware -- they belong in the [VIA Keyboard Repo](https://github.com/the-via/keyboards)
|
- Do not include VIA json files in the PR. These do not belong in the QMK repository as they are not used by QMK firmware -- they belong in the [VIA Keyboard Repo](https://github.com/the-via/keyboards)
|
||||||
|
|
||||||
|
|
||||||
Also, specific to ChibiOS:
|
Also, specific to ChibiOS:
|
||||||
- **strong** preference to using existing ChibiOS board definitions.
|
- **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
|
- 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
|
||||||
|
@ -114,7 +115,7 @@ Also, specific to ChibiOS:
|
||||||
## Core PRs
|
## Core PRs
|
||||||
|
|
||||||
- must now target `develop` branch, which will subsequently be merged back to `master` on the breaking changes timeline
|
- must now target `develop` branch, which will subsequently be merged back to `master` on the breaking changes timeline
|
||||||
- other notes TBD
|
- other requirements are at the discretion of QMK collaborators
|
||||||
- core is a lot more subjective given the breadth of posted changes
|
- core is a lot more subjective given the breadth of posted changes
|
||||||
|
|
||||||
---
|
---
|
||||||
|
@ -159,3 +160,10 @@ Additionally, PR reviews are something that is done in our free time. We are not
|
||||||
* along with this program. If not, see <http://www.gnu.org/licenses/>.
|
* along with this program. If not, see <http://www.gnu.org/licenses/>.
|
||||||
*/
|
*/
|
||||||
```
|
```
|
||||||
|
|
||||||
|
Or, optionally, using [SPDX identifier](https://spdx.org/licenses/) instead:
|
||||||
|
|
||||||
|
```
|
||||||
|
// Copyright 2021 Your Name (@yourgithub)
|
||||||
|
// SPDX-License-Identifier: GPL-2.0-or-later
|
||||||
|
```
|
||||||
|
|
Loading…
Reference in a new issue