Flake8/lint: do not check for style errors that conflict with overall objectives of this project

josephsl commented 5 years ago

Hi,

This proposal is built on top of #9958 work:

Rationale:

lint tools helps a project spot code guidelines before a pull request is merged into a main line branch, thus serving as a contract between project maintainers and outside contributors. However, there are certain coding styles that, although appealing, does not serve the needs of a project. Some style errors should be ignored while linting.

Is your feature request related to a problem? Please describe.

After merging in #9958, several pull requests and their authors have experienced flake8 issues related to coding styles from PEP (Python Enhancement Proposal) 8. These include mixture of tabs and spaces, as well as indentation for visual indent, the latter being a more serious downside.

PEP 8 prescribes coding styles that may not work for this project, and if conflicts occur, project guidelines should take precedence. For example, for ease of readability, NVDA project uses tabs instead of spaces, and maximum line length is 110 characters.

In case of indentation for visual appeal, this is useful for folks who would insist on stricter adherence to PEP 8. However, for people with no or limited sight, it poses problems:

Some editors report odd formatting info for indentations that are mixture of tabs and spaces. For people using these editors, one may think the problematic line was indented correctly when in fact it wasn't.
Some long lines can cause more errors to show up if split. For example, a function with a long name is supposed to read a list of characters, and the problematic line is found to be too long. After splitting it, line length issue may pop up because of the length of the string (partially resolvable by holding this list via a variable). At the same time, tabs/space mixture problem shows up because we're using Python 3.
Due to standardization around tabs, people would assume that one can indent a block by just adding a tab. At least it helps in readability, but PEP 8 says one should use more visually appealing indentation, contrary to the overall objective of this project.

While flake8 is a useful tool for getting people on the same page before submitting a pull request, if not tuned well, it can serve to discourage participation from outsiders who may not understand why NVDA project uses specific guidelines rather than ones prescribed by PEP 8.

Describe the solution you'd like

Thankfully, we have a config file to tune flake8. Thus I propose ignoring the following errors:

E128: continuation line under-indented for visual indent
ET122: (flake8-tabs) unexpected number of spaces at start of expression line
Additional tuning that can conflict with requirements for this project

Describe alternatives you've considered

Leave it as is.

Additional context

See Python Enhancement Proposal 8 for guidelines prescribed for Python code, and NVDA developer guide for guidelines for this project. If there are conflicts, NVDA developer guide should take precedence.

Thanks.

Brian1Gaff commented 5 years ago

Just a comment. This is an old issue that used to affect all formatted texts when you did not want to use a table of course, with some interesting results on other peoples computers where tab was defined differently for printing ove a space. EEK Brian

josephsl commented 5 years ago

Blocked by #10020

feerrenrut commented 5 years ago

Some of this will get easier as we tune the configuration, but it is also a period of adjustment, and an opportunity for people to reconsider the tools or configuration of the tools they are using.

My thoughts on these issues:

Some editors report odd formatting info for indentations that are mixture of tabs and spaces.

I think we should be avoiding mixtures of tabs and spaces.

Some long lines can cause more errors to show up if split.

I think this will get easier after an adjustment period, knowing the patterns to follow to avoid these issues. Long lines can make it easy to miss things that are towards the end, but more often they are a symptom of code that can be broken up into smaller, more manageable pieces.

but PEP 8 says one should use more visually appealing indentation

I don't think this is really required, however, the wording of the errors often don't make it clear there are other options. I have some changes to config pending, once #10020 is merged I am happy to look at examples with you. We may have to create a wiki page with common problems and preferred approaches.

E128: continuation line under-indented for visual indent

See https://lintlyci.github.io/Flake8Rules/rules/E128.html for examples
I don't like the style this encourages, maintaining visual alignment is a pain and a waste of time.
This error can be avoided by putting a newline after the opening bracket.

print("Python", (
    "Hello",
    "World"
))

ET122: (flake8-tabs) unexpected number of spaces at start of expression line

An indentation level in a dictionary for instance helps to let you know you have a closing bracket to come
It ensures you can differentiate between lines that are nested inside some expression and the next line for instance.
Granted, this relies on you being able to easily confirm the current indentation level, however given the importance of indentation in Python I would expect this to be the case anyway.

DrSooom commented 5 years ago

I think we should be avoiding mixtures of tabs and spaces.

I fully agree with this. Furthermore U+0009 can be displayed as ⢀ (dot 8) on a braille display, which makes it very easy to read spaces and tabs correctly. At the moment ⢀ isn't defined in several braille tables, but that's another issue. Workaround: Manually edit the current used braille table – which I did, because I can. 😉

nvaccess / nvda