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

process(python): Explore the possibility of using a single linter #1373

Closed
parthea opened this issue Mar 18, 2022 · 1 comment · Fixed by #1379
Closed

process(python): Explore the possibility of using a single linter #1373

parthea opened this issue Mar 18, 2022 · 1 comment · Fixed by #1379
Labels
lang: python Issues specific to Python. type: process A process-related concern. May include testing, release, or the like.

Comments

@parthea
Copy link
Contributor

parthea commented Mar 18, 2022

In the templated nox_file.py for python, there is a lint nox session which runs both black and flake8 linters.

def lint(session):
"""Run linters.
Returns a failure if the linters find linting errors or sufficiently
serious code quality issues.
"""
session.install("flake8", BLACK_VERSION)
session.run(
"black",
"--check",
*BLACK_PATHS,
)
session.run("flake8", "google", "tests")

googleapis/python-aiplatform#1087, there is a conflict between the 2 lint tools that we use where a formatting change made by black causes the flake8 check to fail.

In the current configuration, black and flake8 check for different things. black tests/applies formatting while flake checks for syntax.

To check this, create an empty file test_lint.py with a single import statement and no new line at the end of the file. As an example, I used import google.api in an empty file without a blank line at the end.

If you run black --check test_lint.py which tests the file without changing it.

black --check test_lint.py
would reformat test_lint.py
All done! 💥 💔 💥
1 file would be reformatted.

Then run black test_lint.py, which adds an empty line at the end of the file.

black test_lint.py
reformatted test_lint.py
All done! ✨ 🍰✨
1 file reformatted.

On the same file run flake8 test_lint.py which tests for syntax.

flake8 test_lint.py
test_lint.py:1:1: F401 'google.api' imported but unused

I've opened this issue to understand the impact of removing black, and confirm that flake8 can fill the gap.

@parthea parthea added type: process A process-related concern. May include testing, release, or the like. lang: python Issues specific to Python. labels Mar 18, 2022
@parthea
Copy link
Contributor Author

parthea commented Mar 30, 2022

I was able to re-create the issue reported in googleapis/python-aiplatform#1087 in this PR. See the build log here. I'm going to open a PR to add E231 to the .flake8 ignore list. That should resolve the issue where the linters are battling with each other. Let's continue to add flake8 formatting errors to the ignore list as needed as we find compatibility issues with black.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang: python Issues specific to Python. type: process A process-related concern. May include testing, release, or the like.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant