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

Jump to bottom

Lint all Python files with flake8 #2933

Closed
conorsch opened this issue Jan 25, 2018 · 2 comments
Closed

Lint all Python files with flake8 #2933

conorsch opened this issue Jan 25, 2018 · 2 comments
Labels
CI-specific bug goals: more tests

Comments

@conorsch
Copy link
Contributor

conorsch commented Jan 25, 2018 •
edited
Loading

Feature request

Description

Our current strategy for linting Python files via flake8 uses a whitelist for file location, which is brittle. If someone adds a new Python file outside of the whitelisted directories, e.g. as in #2922, flake8 will not lint them. That can lead to problems. Specifically, the changes in #2922 did not pass linting and never shold have been merged.

We should update our flake8 linting logic to check all Python files in the repository, which will also catch new additions during CI against PRs.

User Stories

As a developer, I want CI to lint the code I write, not just its favorite code that it's comfortable with.

As a CI pipeline, I want to lint everyone's code, not just the same old stuff I'm used to.
@conorsch
Copy link
Contributor Author

cf. the linting strategy used by make shellcheck, which leverages a blacklist rather than a whitelist. Let's implement that approach, but for Python files.

conorsch pushed a commit that referenced this issue Jan 26, 2018
Fixes broken ansible-config tests
87baf3f 
The tests weren't flake8 compliant, which CI did not caught. There were
two separate tests identically named, so one got clobbered and
effectively we weren't testing both required attributes.

The test file now *does* pass flake8, which I confirmed via manual
invocation against it. Opened a separate issue, #2933, to track
improving CI to avoid this problem in the future.
@conorsch conorsch mentioned this issue Jan 26, 2018
Merged
3 tasks
conorsch pushed a commit that referenced this issue Jan 26, 2018
Fixes broken ansible-config tests
69668db 
The tests weren't flake8 compliant, which CI did not catch. There were
two separate tests identically named, so one got clobbered and
effectively we weren't testing both required attributes.

The test file now *does* pass flake8, which I confirmed via manual
invocation against it. Opened a separate issue, #2933, to track
improving CI to avoid this problem in the future.
@redshiftzero
Copy link
Contributor

We can specify which files not to lint via flake8 --exclude=path/to/files/to/not/lint/*

redshiftzero pushed a commit that referenced this issue Jan 26, 2018
@redshiftzero
Fixes broken ansible-config tests
27adc94 
The tests weren't flake8 compliant, which CI did not catch. There were
two separate tests identically named, so one got clobbered and
effectively we weren't testing both required attributes.

The test file now *does* pass flake8, which I confirmed via manual
invocation against it. Opened a separate issue, #2933, to track
improving CI to avoid this problem in the future.

(cherry picked from commit 69668db)
redshiftzero pushed a commit that referenced this issue Jan 27, 2018
@redshiftzero
Fixes broken ansible-config tests
03ecd34 
The tests weren't flake8 compliant, which CI did not catch. There were
two separate tests identically named, so one got clobbered and
effectively we weren't testing both required attributes.

The test file now *does* pass flake8, which I confirmed via manual
invocation against it. Opened a separate issue, #2933, to track
improving CI to avoid this problem in the future.

(cherry picked from commit 69668db)
@conorsch conorsch mentioned this issue Jan 27, 2018
Merged
3 tasks
@ghost ghost closed this as completed in #2944 Jan 30, 2018
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
CI-specific bug goals: more tests
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@conorsch @redshiftzero