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

Enhance pre-commit hooks with flake8 and black #2407

Merged
merged 3 commits into from
Aug 16, 2024

Conversation

Ygnas
Copy link
Contributor

@Ygnas Ygnas commented Aug 14, 2024

What this PR does / why we need it:
This PR implements the flake8 linter and black formatting as a pre-commit hook to ensure code quality and adherence to style guidelines.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Checklist:

  • Docs included if any changes are user facing

Not sure if I there is a need to exclude something else in the pre-commit config??. If it should be please let me know

@Ygnas Ygnas force-pushed the pc-lint-format branch 2 times, most recently from e438b0c to fb3d11f Compare August 15, 2024 12:42
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this.
Basically, lgtm

@@ -0,0 +1,3 @@
[flake8]
max-line-length = 100
extend-ignore = W503, E203
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
extend-ignore = W503, E203
extend-ignore = W503

Instead of ignoring this error, could you fix those errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure I will, there is a small mistake in my flake8 fix as well, wrong indentation. Will fix that as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Black adds whitespace before : in slices, and flake8 complains about it.

Recomended configs for black:
Compatible configuration files can be found here

This behaviour may raise E203 whitespace before ':' warnings in style guide enforcement tools like Flake8. Since E203 is not PEP 8 compliant, you should tell Flake8 to ignore these warnings. More here.

WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that makes sense.
Could you add a comment here on why we must ignore E203?

Ygnas added 3 commits August 16, 2024 09:10
Also add's the flake8 config file

Signed-off-by: Ignas Baranauskas <ibaranau@redhat.com>
Signed-off-by: Ignas Baranauskas <ibaranau@redhat.com>
Signed-off-by: Ignas Baranauskas <ibaranau@redhat.com>
@Ygnas Ygnas requested a review from tenzen-y August 16, 2024 08:26
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!
/lgtm
/approve

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tenzen-y

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit e9e6e0c into kubeflow:master Aug 16, 2024
63 checks passed
Comment on lines +16 to +23
hooks:
- id: black
files: (sdk|examples)/.*
- repo: https://github.com/pycqa/flake8
rev: 7.1.1
hooks:
- id: flake8
files: (sdk|examples)/.*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

@helenxie-bit
Copy link
Contributor

@Ygnas Thank you for your contribution! I'm trying to update a PR (#2393), and the new pre-commit check is working. However, it looks like there's an issue with the compatibility between the isort and black checks. Specifically, I've run "pre-commit run --all-files" locally, but the isort and black checks keep failing for "katib_client.py". It seems that this is because the pre-commit configuration is using the --profile google argument with isort, which has a different style than black. Could you please take a look into this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants