From e93b85c4f2ecd18552ce2bd191f1e6cdcba6f020 Mon Sep 17 00:00:00 2001 From: skullY Date: Sun, 25 Oct 2020 10:56:50 -0700 Subject: [PATCH 01/10] Basic qmk lint command --- lib/python/qmk/cli/__init__.py | 1 + lib/python/qmk/cli/lint.py | 60 ++++++++++++++++++++++++++++++++++ lib/python/qmk/info.py | 43 +++++++++++++++++------- lib/python/qmk/path.py | 14 +++++--- 4 files changed, 102 insertions(+), 16 deletions(-) create mode 100644 lib/python/qmk/cli/lint.py diff --git a/lib/python/qmk/cli/__init__.py b/lib/python/qmk/cli/__init__.py index 47e1b443518a..f99c36f630e2 100644 --- a/lib/python/qmk/cli/__init__.py +++ b/lib/python/qmk/cli/__init__.py @@ -18,6 +18,7 @@ from . import info from . import json from . import json2c +from . import lint from . import list from . import kle2json from . import new diff --git a/lib/python/qmk/cli/lint.py b/lib/python/qmk/cli/lint.py new file mode 100644 index 000000000000..8ae86341651a --- /dev/null +++ b/lib/python/qmk/cli/lint.py @@ -0,0 +1,60 @@ +"""Command to look over a keyboard/keymap and check for common mistakes. +""" +from milc import cli + +from qmk.decorators import automagic_keyboard, automagic_keymap +from qmk.info import info_json +from qmk.keymap import locate_keymap +from qmk.path import is_keyboard, keyboard + + +@cli.argument('-kb', '--keyboard', help='The keyboard to check.') +@cli.argument('-km', '--keymap', help='The keymap to check.') +@cli.subcommand('Check keyboard and keymap for common mistakes.') +@automagic_keyboard +@automagic_keymap +def lint(cli): + """Check keyboard and keymap for common mistakes. + """ + if not cli.config.lint.keyboard: + cli.log.error('Could not determine keyboard!') + print() + cli.print_help() + return False + + if not is_keyboard(cli.config.lint.keyboard): + cli.log.error('No such keyboard: %s', cli.config.lint.keyboard) + return False + + # Gather data about the keyboard. + ok = True + keyboard_path = keyboard(cli.config.lint.keyboard) + keyboard_info = info_json(cli.config.lint.keyboard) + readme_path = keyboard_path / 'readme.md' + + # Check for errors in the info.json + if keyboard_info['parsing_errors']: + cli.log.error('Errors found when generating info.json.') + ok = False + + # Check for a readme.md and warn if it doesn't exist + if not readme_path.exists(): + ok = False + cli.log.error('Missing %s', readme_path) + + # Keymap specific checks + if cli.config.lint.keymap: + keymap_path = locate_keymap(cli.config.lint.keyboard, cli.config.lint.keymap) + if not keymap_path: + ok = False + cli.log.error("Can't find %s keymap for %s keyboard.", cli.config.lint.keymap, cli.config.lint.keyboard) + else: + pass + + # Check and report the overall status + if ok: + cli.log.info('Lint check passed!') + return True + + cli.log.error('Lint check failed!') + return False diff --git a/lib/python/qmk/info.py b/lib/python/qmk/info.py index e92c3335b98b..d0d3f2283722 100644 --- a/lib/python/qmk/info.py +++ b/lib/python/qmk/info.py @@ -28,6 +28,7 @@ def info_json(keyboard): 'keyboard_folder': str(keyboard), 'keymaps': {}, 'layouts': {}, + 'parsing_errors': [], 'maintainer': 'qmk', } @@ -36,7 +37,7 @@ def info_json(keyboard): info_data['keymaps'][keymap.name] = {'url': f'https://raw.githubusercontent.com/qmk/qmk_firmware/master/{keymap}/keymap.json'} # Populate layout data - for layout_name, layout_json in _find_all_layouts(keyboard, rules).items(): + for layout_name, layout_json in _find_all_layouts(info_data, keyboard, rules).items(): if not layout_name.startswith('LAYOUT_kc'): info_data['layouts'][layout_name] = layout_json @@ -104,14 +105,17 @@ def _extract_rules_mk(info_data): mcu = rules.get('MCU') if mcu in CHIBIOS_PROCESSORS: - arm_processor_rules(info_data, rules) + return arm_processor_rules(info_data, rules) + elif mcu in LUFA_PROCESSORS + VUSB_PROCESSORS: - avr_processor_rules(info_data, rules) - else: - cli.log.warning("%s: Unknown MCU: %s" % (info_data['keyboard_folder'], mcu)) - unknown_processor_rules(info_data, rules) + return avr_processor_rules(info_data, rules) - return info_data + msg = "%s: Unknown MCU: %s" % (info_data['keyboard_folder'], mcu) + + info_data['parsing_errors'].append(msg) + _log_warning(info_data, msg) + + return unknown_processor_rules(info_data, rules) def _search_keyboard_h(path): @@ -127,7 +131,7 @@ def _search_keyboard_h(path): return layouts -def _find_all_layouts(keyboard, rules): +def _find_all_layouts(info_data, keyboard, rules): """Looks for layout macros associated with this keyboard. """ layouts = _search_keyboard_h(Path(keyboard)) @@ -135,7 +139,7 @@ def _find_all_layouts(keyboard, rules): if not layouts: # If we didn't find any layouts above we widen our search. This is error # prone which is why we want to encourage people to follow the standard above. - cli.log.warning('%s: Falling back to searching for KEYMAP/LAYOUT macros.' % (keyboard)) + _log_warning(info_data, '%s: Falling back to searching for KEYMAP/LAYOUT macros.' % (keyboard)) for file in glob('keyboards/%s/*.h' % keyboard): if file.endswith('.h'): these_layouts = find_layouts(file) @@ -153,11 +157,25 @@ def _find_all_layouts(keyboard, rules): supported_layouts.remove(layout_name) if supported_layouts: - cli.log.error('%s: Missing LAYOUT() macro for %s' % (keyboard, ', '.join(supported_layouts))) + _log_error(info_data, '%s: Missing LAYOUT() macro for %s' % (keyboard, ', '.join(supported_layouts))) return layouts +def _log_error(info_data, message): + """Send an error message to both JSON and the log. + """ + info_data['parsing_errors'].append(message) + cli.log.error(message) + + +def _log_warning(info_data, message): + """Send a warning message to both JSON and the log. + """ + info_data['parsing_errors'].append(message) + cli.log.warning(message) + + def arm_processor_rules(info_data, rules): """Setup the default info for an ARM board. """ @@ -216,7 +234,7 @@ def merge_info_jsons(keyboard, info_data): new_info_data = json.load(info_fd) if not isinstance(new_info_data, dict): - cli.log.error("Invalid file %s, root object should be a dictionary.", str(info_file)) + _log_error(info_data, "Invalid file %s, root object should be a dictionary.", str(info_file)) continue # Copy whitelisted keys into `info_data` @@ -230,7 +248,8 @@ def merge_info_jsons(keyboard, info_data): # Only pull in layouts we have a macro for if layout_name in info_data['layouts']: if info_data['layouts'][layout_name]['key_count'] != len(json_layout['layout']): - cli.log.error('%s: %s: Number of elements in info.json does not match! info.json:%s != %s:%s', info_data['keyboard_folder'], layout_name, len(json_layout['layout']), layout_name, len(info_data['layouts'][layout_name]['layout'])) + msg = '%s: %s: Number of elements in info.json does not match! info.json:%s != %s:%s' + _log_error(info_data, msg % (info_data['keyboard_folder'], layout_name, len(json_layout['layout']), layout_name, len(info_data['layouts'][layout_name]['layout']))) else: for i, key in enumerate(info_data['layouts'][layout_name]['layout']): key.update(json_layout['layout'][i]) diff --git a/lib/python/qmk/path.py b/lib/python/qmk/path.py index 591fad034b21..54def1d5d6cc 100644 --- a/lib/python/qmk/path.py +++ b/lib/python/qmk/path.py @@ -28,15 +28,21 @@ def under_qmk_firmware(): return None -def keymap(keyboard): +def keyboard(keyboard_name): + """Returns the path to a keyboard's directory relative to the qmk root. + """ + return Path('keyboards') / keyboard_name + + +def keymap(keyboard_name): """Locate the correct directory for storing a keymap. Args: - keyboard + keyboard_name The name of the keyboard. Example: clueboard/66/rev3 """ - keyboard_folder = Path('keyboards') / keyboard + keyboard_folder = keyboard(keyboard_name) for i in range(MAX_KEYBOARD_SUBFOLDERS): if (keyboard_folder / 'keymaps').exists(): @@ -45,7 +51,7 @@ def keymap(keyboard): keyboard_folder = keyboard_folder.parent logging.error('Could not find the keymaps directory!') - raise NoSuchKeyboardError('Could not find keymaps directory for: %s' % keyboard) + raise NoSuchKeyboardError('Could not find keymaps directory for: %s' % keyboard_name) def normpath(path): From a1ff3b265751339b338868b205aaefd0fb5ee4a8 Mon Sep 17 00:00:00 2001 From: skullY Date: Sun, 25 Oct 2020 11:46:20 -0700 Subject: [PATCH 02/10] check for keymap readme --- lib/python/qmk/cli/lint.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/python/qmk/cli/lint.py b/lib/python/qmk/cli/lint.py index 8ae86341651a..8490205c125a 100644 --- a/lib/python/qmk/cli/lint.py +++ b/lib/python/qmk/cli/lint.py @@ -17,8 +17,7 @@ def lint(cli): """Check keyboard and keymap for common mistakes. """ if not cli.config.lint.keyboard: - cli.log.error('Could not determine keyboard!') - print() + cli.log.error('Missing required argument: --keyboard') cli.print_help() return False @@ -45,11 +44,14 @@ def lint(cli): # Keymap specific checks if cli.config.lint.keymap: keymap_path = locate_keymap(cli.config.lint.keyboard, cli.config.lint.keymap) + if not keymap_path: ok = False cli.log.error("Can't find %s keymap for %s keyboard.", cli.config.lint.keymap, cli.config.lint.keyboard) else: - pass + keymap_readme = keymap_path.parent / 'readme.md' + if not keymap_readme.exists(): + cli.log.warning('Missing %s', keymap_readme) # Check and report the overall status if ok: From 4d5b4576871f6bae05e7cfdccc7b1c3cb537cf77 Mon Sep 17 00:00:00 2001 From: Zach White Date: Thu, 29 Oct 2020 07:54:57 -0700 Subject: [PATCH 03/10] change the workflow from qmk info to qmk lint --- .github/workflows/{info.yml => lint.yml} | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) rename .github/workflows/{info.yml => lint.yml} (80%) diff --git a/.github/workflows/info.yml b/.github/workflows/lint.yml similarity index 80% rename from .github/workflows/info.yml rename to .github/workflows/lint.yml index bb3a50847766..e94054ced606 100644 --- a/.github/workflows/info.yml +++ b/.github/workflows/lint.yml @@ -27,7 +27,7 @@ jobs: echo ${{ github.event.pull_request.base.sha }} echo '${{ steps.file_changes.outputs.files}}' - - name: Run qmk info + - name: Run qmk lint shell: 'bash {0}' run: | QMK_CHANGES=$(echo -e '${{ steps.file_changes.outputs.files}}') @@ -45,10 +45,7 @@ jobs: if [[ $KEYMAP_ONLY -gt 0 ]]; then echo "linting ${KB}" - # TODO: info info always returns 0 - right now the only way to know failure is to inspect log lines - qmk info -l -kb ${KB} 2>&1 | tee /tmp/$$ - !(grep -cq ☒ /tmp/$$) - : $((exit_code = $exit_code + $?)) + qmk lint --keyboard ${KB} fi done exit $exit_code From 263e266a8854bd07482dc56cc1bc0cee5ece294c Mon Sep 17 00:00:00 2001 From: Zach White Date: Thu, 29 Oct 2020 08:10:27 -0700 Subject: [PATCH 04/10] add a strict mode --- lib/python/qmk/cli/lint.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/python/qmk/cli/lint.py b/lib/python/qmk/cli/lint.py index 8490205c125a..80c97a44205d 100644 --- a/lib/python/qmk/cli/lint.py +++ b/lib/python/qmk/cli/lint.py @@ -8,6 +8,7 @@ from qmk.path import is_keyboard, keyboard +@cli.argument('--strict', action='store_true', help='Treat warnings as errors.') @cli.argument('-kb', '--keyboard', help='The keyboard to check.') @cli.argument('-km', '--keymap', help='The keymap to check.') @cli.subcommand('Check keyboard and keymap for common mistakes.') @@ -33,8 +34,12 @@ def lint(cli): # Check for errors in the info.json if keyboard_info['parsing_errors']: + ok = False cli.log.error('Errors found when generating info.json.') + + if cli.config.lint.strict and keyboard_info['parsing_warnings']: ok = False + cli.log.error('Warnings found when generating info.json (Strict mode enabled.)') # Check for a readme.md and warn if it doesn't exist if not readme_path.exists(): From 9ad6ceacc67dffb0d77c56297efb09f4acf6c9df Mon Sep 17 00:00:00 2001 From: Zach White Date: Thu, 29 Oct 2020 08:13:36 -0700 Subject: [PATCH 05/10] parsing -> parse --- lib/python/qmk/cli/lint.py | 4 ++-- lib/python/qmk/info.py | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/python/qmk/cli/lint.py b/lib/python/qmk/cli/lint.py index 80c97a44205d..2ccfa2543873 100644 --- a/lib/python/qmk/cli/lint.py +++ b/lib/python/qmk/cli/lint.py @@ -33,11 +33,11 @@ def lint(cli): readme_path = keyboard_path / 'readme.md' # Check for errors in the info.json - if keyboard_info['parsing_errors']: + if keyboard_info['parse_errors']: ok = False cli.log.error('Errors found when generating info.json.') - if cli.config.lint.strict and keyboard_info['parsing_warnings']: + if cli.config.lint.strict and keyboard_info['parse_warnings']: ok = False cli.log.error('Warnings found when generating info.json (Strict mode enabled.)') diff --git a/lib/python/qmk/info.py b/lib/python/qmk/info.py index d0d3f2283722..5379b82250e5 100644 --- a/lib/python/qmk/info.py +++ b/lib/python/qmk/info.py @@ -28,7 +28,8 @@ def info_json(keyboard): 'keyboard_folder': str(keyboard), 'keymaps': {}, 'layouts': {}, - 'parsing_errors': [], + 'parse_errors': [], + 'parse_warnings': [], 'maintainer': 'qmk', } @@ -112,7 +113,6 @@ def _extract_rules_mk(info_data): msg = "%s: Unknown MCU: %s" % (info_data['keyboard_folder'], mcu) - info_data['parsing_errors'].append(msg) _log_warning(info_data, msg) return unknown_processor_rules(info_data, rules) @@ -165,14 +165,14 @@ def _find_all_layouts(info_data, keyboard, rules): def _log_error(info_data, message): """Send an error message to both JSON and the log. """ - info_data['parsing_errors'].append(message) + info_data['parse_errors'].append(message) cli.log.error(message) def _log_warning(info_data, message): """Send a warning message to both JSON and the log. """ - info_data['parsing_errors'].append(message) + info_data['parse_warnings'].append(message) cli.log.warning(message) From 24b860d9a9ca48727fb2c4e9719a147c84eb0a26 Mon Sep 17 00:00:00 2001 From: Zach White Date: Thu, 29 Oct 2020 08:23:17 -0700 Subject: [PATCH 06/10] document qmk lint --- docs/cli_commands.md | 18 ++++++++++++++++++ docs/hardware_keyboard_guidelines.md | 19 +++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/docs/cli_commands.md b/docs/cli_commands.md index fe6f06632a9c..be5890cbc228 100644 --- a/docs/cli_commands.md +++ b/docs/cli_commands.md @@ -178,6 +178,24 @@ Creates a keymap.json from a keymap.c. qmk c2json [--no-cpp] [-o OUTPUT] filename ``` +## `qmk lint` + +Checks over a keyboard and/or keymap and highlights common errors, problems, and anti-patterns. + +**Usage**: + +``` +qmk lint [-km KEYMAP] [-kb KEYBOARD] [--strict] +``` + +This command is directory aware. It will automatically fill in KEYBOARD and/or KEYMAP if you are in a keyboard or keymap directory. + +**Examples**: + +Do a basic lint check: + + qmk info -kb rominronin/katana60/rev2 + ## `qmk list-keyboards` This command lists all the keyboards currently defined in `qmk_firmware` diff --git a/docs/hardware_keyboard_guidelines.md b/docs/hardware_keyboard_guidelines.md index d49d0d092807..37baea65e37e 100644 --- a/docs/hardware_keyboard_guidelines.md +++ b/docs/hardware_keyboard_guidelines.md @@ -3,6 +3,25 @@ Since starting, QMK has grown by leaps and bounds thanks to people like you who contribute to creating and maintaining our community keyboards. As we've grown we've discovered some patterns that work well, and ask that you conform to them to make it easier for other people to benefit from your hard work. +## Use QMK Lint + +We have provided a tools, `qmk lint`, which will let you check over your keyboard for problems. We suggest using it frequently while working on your keyboard and keymap. + +Example passing check: + +``` +$ qmk lint -kb rominronin/katana60/rev2 +Ψ Lint check passed! +``` + +Example failing check: + +``` +$ qmk lint -kb clueboard/66/rev3 +☒ Missing keyboards/clueboard/66/rev3/readme.md +☒ Lint check failed! +``` + ## Naming Your Keyboard/Project All keyboard names are in lower case, consisting only of letters, numbers, and underscore (`_`). Names may not begin with an underscore. Forward slash (`/`) is used as a sub-folder separation character. From fe7e49b9a957505ef148767a5ca7007d6bd64000 Mon Sep 17 00:00:00 2001 From: Zach White Date: Thu, 29 Oct 2020 08:34:56 -0700 Subject: [PATCH 07/10] small info logging cleanup --- lib/python/qmk/info.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/python/qmk/info.py b/lib/python/qmk/info.py index 5379b82250e5..d73ba8cfb61a 100644 --- a/lib/python/qmk/info.py +++ b/lib/python/qmk/info.py @@ -111,7 +111,7 @@ def _extract_rules_mk(info_data): elif mcu in LUFA_PROCESSORS + VUSB_PROCESSORS: return avr_processor_rules(info_data, rules) - msg = "%s: Unknown MCU: %s" % (info_data['keyboard_folder'], mcu) + msg = "Unknown MCU: " + str(mcu) _log_warning(info_data, msg) @@ -139,7 +139,7 @@ def _find_all_layouts(info_data, keyboard, rules): if not layouts: # If we didn't find any layouts above we widen our search. This is error # prone which is why we want to encourage people to follow the standard above. - _log_warning(info_data, '%s: Falling back to searching for KEYMAP/LAYOUT macros.' % (keyboard)) + _log_warning(info_data, 'Falling back to searching for KEYMAP/LAYOUT macros.') for file in glob('keyboards/%s/*.h' % keyboard): if file.endswith('.h'): these_layouts = find_layouts(file) @@ -157,7 +157,7 @@ def _find_all_layouts(info_data, keyboard, rules): supported_layouts.remove(layout_name) if supported_layouts: - _log_error(info_data, '%s: Missing LAYOUT() macro for %s' % (keyboard, ', '.join(supported_layouts))) + _log_error(info_data, 'Missing LAYOUT() macro for %s' % (', '.join(supported_layouts))) return layouts @@ -166,14 +166,14 @@ def _log_error(info_data, message): """Send an error message to both JSON and the log. """ info_data['parse_errors'].append(message) - cli.log.error(message) + cli.log.error('%s: %s', info_data.get('keyboard_folder', 'Unknown Keyboard!'), message) def _log_warning(info_data, message): """Send a warning message to both JSON and the log. """ info_data['parse_warnings'].append(message) - cli.log.warning(message) + cli.log.warning('%s: %s', info_data.get('keyboard_folder', 'Unknown Keyboard!'), message) def arm_processor_rules(info_data, rules): @@ -248,8 +248,8 @@ def merge_info_jsons(keyboard, info_data): # Only pull in layouts we have a macro for if layout_name in info_data['layouts']: if info_data['layouts'][layout_name]['key_count'] != len(json_layout['layout']): - msg = '%s: %s: Number of elements in info.json does not match! info.json:%s != %s:%s' - _log_error(info_data, msg % (info_data['keyboard_folder'], layout_name, len(json_layout['layout']), layout_name, len(info_data['layouts'][layout_name]['layout']))) + msg = '%s: Number of elements in info.json does not match! info.json:%s != %s:%s' + _log_error(info_data, msg % (layout_name, len(json_layout['layout']), layout_name, len(info_data['layouts'][layout_name]['layout']))) else: for i, key in enumerate(info_data['layouts'][layout_name]['layout']): key.update(json_layout['layout'][i]) From b23bc073ef19a6f3c0e0a0728fc5f5d0083b8c7b Mon Sep 17 00:00:00 2001 From: Zach White Date: Thu, 29 Oct 2020 11:50:11 -0700 Subject: [PATCH 08/10] Apply suggestions from code review Co-authored-by: Ryan --- docs/cli_commands.md | 3 +-- docs/hardware_keyboard_guidelines.md | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/docs/cli_commands.md b/docs/cli_commands.md index be5890cbc228..4e81fdcb0549 100644 --- a/docs/cli_commands.md +++ b/docs/cli_commands.md @@ -194,7 +194,7 @@ This command is directory aware. It will automatically fill in KEYBOARD and/or K Do a basic lint check: - qmk info -kb rominronin/katana60/rev2 + qmk lint -kb rominronin/katana60/rev2 ## `qmk list-keyboards` @@ -317,4 +317,3 @@ This command runs the python test suite. If you make changes to python code you ``` qmk pytest ``` - diff --git a/docs/hardware_keyboard_guidelines.md b/docs/hardware_keyboard_guidelines.md index 37baea65e37e..742b56572ca4 100644 --- a/docs/hardware_keyboard_guidelines.md +++ b/docs/hardware_keyboard_guidelines.md @@ -5,7 +5,7 @@ Since starting, QMK has grown by leaps and bounds thanks to people like you who ## Use QMK Lint -We have provided a tools, `qmk lint`, which will let you check over your keyboard for problems. We suggest using it frequently while working on your keyboard and keymap. +We have provided a tool, `qmk lint`, which will let you check over your keyboard for problems. We suggest using it frequently while working on your keyboard and keymap. Example passing check: From 5f2d2bf4ec252594c0321a261d4c579db612117f Mon Sep 17 00:00:00 2001 From: Zach White Date: Thu, 29 Oct 2020 17:54:08 -0700 Subject: [PATCH 09/10] honor --strict in more places --- lib/python/qmk/cli/lint.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/python/qmk/cli/lint.py b/lib/python/qmk/cli/lint.py index 2ccfa2543873..74467021e075 100644 --- a/lib/python/qmk/cli/lint.py +++ b/lib/python/qmk/cli/lint.py @@ -58,6 +58,9 @@ def lint(cli): if not keymap_readme.exists(): cli.log.warning('Missing %s', keymap_readme) + if cli.config.lint.strict: + ok = False + # Check and report the overall status if ok: cli.log.info('Lint check passed!') From 79ab2738a0f0ddeed4b6aa688343e8f926df0833 Mon Sep 17 00:00:00 2001 From: Zach White Date: Thu, 29 Oct 2020 17:54:48 -0700 Subject: [PATCH 10/10] change the job name to lint --- .github/workflows/lint.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index e94054ced606..1aa347a1b2fd 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -6,7 +6,7 @@ on: - 'keyboards/**' jobs: - info: + lint: runs-on: ubuntu-latest container: qmkfm/base_container