From 70851728fe2b02f6486b83b76dcd083c99755311 Mon Sep 17 00:00:00 2001 From: Reef Turner Date: Mon, 12 Aug 2019 19:30:02 +0200 Subject: [PATCH] Guide: address common lint errors (#10050) Developers are having trouble to know how to resolve certain lint errors. Specifically misleading is ET128 (flake8-tabs) unexpected number of tabs and spaces at start of expression/definition line. It is likely that flake8 is expecting a code alignment style, rather than hanging indent style in this case. To make it expect hanging indent, first ensure that there is a newline after the opening paren/bracket/brace. --- readme.md | 1 + tests/lint/readme.md | 111 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+) diff --git a/readme.md b/readme.md index 06c18b4295d..bb7b2758ff2 100644 --- a/readme.md +++ b/readme.md @@ -256,6 +256,7 @@ scons checkPot ### Linting your changes In order to ensure your changes comply with NVDA's coding style you can run the Flake8 linter locally. +Some developers have found certain linting error messages misleading, these are clarified in `tests/lint/readme.md`. Running via SCons will use Flake8 to inspect only the differences between your working directory and the specified `base` branch. If you create a Pull Request, the `base` branch you use here should be the same as the target you would use for a Pull Request. In most cases it will be `origin/master`. ``` diff --git a/tests/lint/readme.md b/tests/lint/readme.md index 965dd8072c1..17997c60855 100644 --- a/tests/lint/readme.md +++ b/tests/lint/readme.md @@ -4,6 +4,117 @@ Our linting process with Flake8 consists of two main steps: - Generating a diff - Running Flake8 on the diff +## Common problems +There are several common situations for which the Flake8 errors don't clearly indicate an error free solution. + +### Continuation lines + +According to the +[Pep8 indentation guide](https://www.python.org/dev/peps/pep-0008/#indentation), +continuation lines (Python's implicit line joining inside parentheses) are expected +to follow one of two main styles: + +- Vertical alignment + - The first parameter is on the same line as the opening parenthesis + - For each subsequent line, the first character should be aligned with the + first character of the first parameter. +- Hanging indent + - There is no parameter on the same line as the opening parenthesis. + - The first parameter is indented by a standard amount on the following line. + - All subsequent parameters have the same indent. + +What this means for us: +- Vertical alignment + - Requires spaces to meet the length of arbitrary function/variable names + - Requires counting characters to determine the number of spaces before arguments + - Alignment must be changed if the function/variable name changes. +- Hanging indent + - Fixed indentation + - Takes up more vertical space + +The Flake8 checkers don't know which style we intend, they allow for both. It guesses +which one you want based on the location of the first token after the opening parenthesis. +The expected indentation it suggests can cause confusion, especially if the indentation is +as expected, but the first param/condition/item is on the same line as the opening parenthesis. + +#### Preferred formatting for continuation lines + +- Use hanging indent style. + - Line break after the opening parenthesis, putting the first parameter on a new line. +- For function definitions, double indent the params to avoid ET121 + +```python +# method with many parameters +# use "hanging indent style" - start params on new line to avoid ET128 +def foo( + arg1, # double indent to avoid ET121 + arg2 +): # put the closing paren on a new line, reduce the diff when changing parameters. + # long expression + # use "hanging indent style" - start params on new line to avoid ET128 + if( + arg1 is not None # not a function definition, no double indent required + and arg2 is None + ): # put the closing paren on a new line, reduce the diff when changing conditions + return None + + # use "hanging indent style" - start params on new line to avoid ET128 + values = [ + "value1", # not a function definition, no double indent required + "value2", + ] # put the closing bracket on a new line, reduce the diff when adding items. + return values + +``` + +Note: An inline comment on an opening parenthesis/bracket/brace causes an +erroneous message from flake8-tabs. See https://gitlab.com/ntninja/flake8-tabs/issues/1 +EG: +```python +def foo( # a comment here causes error ET128 + arg1 +): + items = [ # a comment here causes error ET128 + "item1", + "item2", + ] +``` + +#### ET128 (flake8-tabs) + +Error messages: +- *unexpected number of tabs at start of definition line* +- *unexpected number of tabs and spaces at start of expression line*s + +Its likely that this is triggered because the linter is expecting "vertical alignment" +style for the set of continuation lines, rather than "hanging indent" style. To change this, +ensure that there is a newline after the opening paren/bracket/brace. + +An example cause: +```python +def foo(arg1, + arg2, # arg2 not vertically aligned with start of first parameter. +): + return None +``` + +#### ET121 (flake8-tabs) + +Error messages: +- *unexpected number of tabs at start of definition line (expected 2, got 1)* + +Example cause: +```python +def foo( + arg1, # one level of indentation, matches the function body + arg2, +): + return None +``` + +In function definitions we require double indentation of parameters to differentiate from +the body of the function. + ## Scons lint Executed with SCons. ```