Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More PR checklist updates #14705

Merged
merged 2 commits into from
Oct 5, 2021
Merged

More PR checklist updates #14705

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
535132c
Wording, clarification.
tzarc Oct 5, 2021
260be6a
Apply suggestions from code review
tzarc Oct 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 21 additions & 13 deletions docs/pr_checklist.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).

## General PRs
## Requirements for all 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
- 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 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
- valid license headers on all `*.c` and `*.h` source files
- GPL2/GPL3 recommended for consistency
Expand All @@ -21,7 +21,7 @@ If there are any inconsistencies with these recommendations, you're best off [cr
- 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)
- 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:
- `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
Expand All @@ -30,7 +30,7 @@ If there are any inconsistencies with these recommendations, you're best off [cr
- discuss it with QMK Collaborators on Discord
- refactor it as a separate core change
- 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

Expand All @@ -50,11 +50,13 @@ https://github.com/qmk/qmk_firmware/pulls?q=is%3Apr+is%3Aclosed+label%3Akeyboard
- 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
- standard template should be present -- [link to template](https://github.com/qmk/qmk_firmware/blob/master/data/templates/avr/readme.md)
- 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.
- clear instructions on how to reset the board into bootloader mode
- 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/).
Copy link
Member

@daskygit daskygit Oct 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a little bit of a nitpick but do they have to be on a image hosting service or is self hosting ok?

EDIT: I should a little clearer, can the images be hosted at myownfancydomain.com for example?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For longevity, I'd want to avoid people personally hosting images. With larger hosts like imgur, at least we'll know ahead of time if they're shutting down (I would hope), and be able to migrate elsewhere.

- `rules.mk`
- removed `MIDI_ENABLE`, `FAUXCLICKY_ENABLE` and `HD44780_ENABLE`
- modified `# Enable Bluetooth with the Adafruit EZ-Key HID` -> `# Enable Bluetooth`
Expand All @@ -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
- 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)
- `keyboard.c`
- `<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)
- 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
- `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)
- `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...
```
layer_state_t layer_state_set_user(layer_state_t state) {
Expand All @@ -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
- 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:
- **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
Expand All @@ -114,7 +115,7 @@ Also, specific to ChibiOS:
## Core PRs

- 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

---
Expand All @@ -136,7 +137,7 @@ Thanks for contributing!

## Review Process

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!
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.

Expand All @@ -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/>.
*/
```

Or, optionally, using [SPDX identifier](https://spdx.org/licenses/) instead:

```
// Copyright 2021 Your Name (@yourgithub)
// SPDX-License-Identifier: GPL-2.0-or-later
```