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.

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

Guide: address common lint errors #10050

Merged
merged 3 commits into from Aug 12, 2019

Conversation

@feerrenrut
Copy link
Contributor

commented Aug 8, 2019

Link to issue number:

None

Summary of the issue:

Several reports of developers having trouble to know how to resolve lint errors.

Specifically misleading is ET128 (flake8-tabs) unexpected number of tabs and spaces at start of expression line. When this error message says "expects x spaces", its likely that it is expecting a code alignment style, rather than hanging indent style. To change this expectation, first ensure that there is a newline after the opening paren/bracket/brace.

Description of how this pull request fixes the issue:

Updates the tests/lint/readme.md with explanation and examples.
Update the wiki contributing guide: https://github.com/nvaccess/nvda/wiki/Contributing

Also raised an issue with the flake8-tabs project: https://gitlab.com/ntninja/flake8-tabs/issues/1

Testing performed:

None

Known issues with pull request:

None

Change log entry:

None

@feerrenrut feerrenrut requested review from leonardder and jcsteh Aug 8, 2019

@leonardder
Copy link
Collaborator

left a comment

I think this documentation will be very helpful. May be also note in the base readme that the linting documentation also contains a how to on dealing with common linting errors that are easy to understand/resolve.

tests/lint/readme.md Outdated Show resolved Hide resolved
tests/lint/readme.md Outdated Show resolved Hide resolved
tests/lint/readme.md Outdated Show resolved Hide resolved
tests/lint/readme.md Outdated Show resolved Hide resolved
@feerrenrut

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

@leonardder I did a bit of a re-write, I think it is much clearer now. I hope you think so too. I think part of the difficulty here is that there are several terms with subtly differences to learn. Hopefully the changes I've made cover this better.

@leonardder
Copy link
Collaborator

left a comment

This is awesome.

Have you considered my comment about adding an extra hint to the base readme of the repo to note that the lint readme contains common errors to deal with?

tests/lint/readme.md Outdated Show resolved Hide resolved

@feerrenrut feerrenrut merged commit 7085172 into master Aug 12, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Aug 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.