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 against this document, or getting in touch with a QMK Collaborator on Discord.
master
branch on the source repository
master
branch, they'll be given a link to the "how to git" page after merging -- (end of this document will contain the contents of the message)*.c
and *.h
source files
#pragma once
instead of #ifndef
include guards in header fileswait_ms()
instead of _delay_ms()
(remove #include <util/delay.h>
too)timer_read()
and timer_read32()
etc. -- see timer.h for the timing APIs#include QMK_KEYBOARD_H
preferred to including specific board filesenum
s to #define
senum
s to #define
s, first entry must have = SAFE_RANGE
\
) in lines of LAYOUT macro parameters is superfluousClosed 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
readme.md
:flash
at endrules.mk
MIDI_ENABLE
, FAUXCLICKY_ENABLE
and HD44780_ENABLE
# Enable Bluetooth with the Adafruit EZ-Key HID
-> # Enable Bluetooth
(-/+size)
comments related to enabling featuresconfig.h
MANUFACTURER
in the PRODUCT
value#define DESCRIPTION
#define
s need to be moved to keymap config.h
DEBOUNCE
" instead of "DEBOUNCING_DELAY
"default
keymapskeyboard.c
xxxx_xxxx_kb()
or other weak-defined default implemented functions removedmatrix_init_board()
etc. migrated to keyboard_pre_init_kb()
, see: keyboard_pre_init*CUSTOM_MATRIX = lite
if custom matrix used, allows for standard debounce, see custom matrix 'lite'led_update_*()
implementations where possiblekeyboard.h
#include "quantum.h"
appears at the topLAYOUT
macros should use standard definitions if applicable
LAYOUT
/LAYOUT_all
)config.h
rules.mk
or config.h
from keyboardkeymaps/default/keymap.c
QMKBEST
/QMKURL
removed (sheesh)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()
.
Also, specific to ChibiOS:
BOARD = ST_NUCLEO64_L073RZ
in rules.mkboard.c
must have a standard __early_init()
(as per normal ChibiOS board defs) and an empty boardInit()
:
__early_init()
should be replaced by either early_hardware_init_pre()
or early_hardware_init_post()
as appropriateboardInit()
should be migrated to board_init()
develop
branch, which will subsequently be merged back to master
on the breaking changes timelineFor 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!
In general, we want to see two (or more) approvals that are meaningful (e.g. that have inspected code) before a PR will be considered for merge. These reviews are not limited to collaborators -- any community member willing to put in the time is welcomed (and encouraged). The only difference is that your checkmark won't be green, and that's fine!
Additionally, PR reviews are something that is done in our free time. We are not paid nor compensated for the time we spend reviewing, as it is a labor of love. As such, this means that it can take time for us to get to your Pull Request. Things like family, or life can get in the way of us getting to PRs, and burnout is a serious concern. The QMK firmware repository averages 200 PRs opened and 200 PRs merged every month, so please have patience.