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

Linter integration #1143

Merged
merged 4 commits into from
Jul 4, 2022

Conversation

yalcinmelihyasin
Copy link
Contributor

Integrate Autopep8, Flake8 and mypy to our build system.

name = "tests-vulkan-generator",
tests = [
# Run the linting as part of Vulkan generator tests
"//:lint", # DO NOT REMOVE!
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a comment saying do not remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it is very easy to remove a test from bazel and not catch its missing. I just want to ensure that no one comes and says "why this is here" and remove it without thinking the implications. Having it there has no harm.

TARGETS=$(echo "$TARGETS" | grep -v "_flake8" | grep -v "_mypy" | grep -v "_pylint")

# Sort the linting targets alphabetically
TARGETS=$(echo "$TARGETS" | sort -t: -k1,1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we factor this common stuff out somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thought of but almost every function there is a singular presubmit case. So the benefit of splitting it have not justified break the pattern for me. Instead I put them in a way to make the process clearer for both case.



if __name__ == "__main__":
flake8_cli.main(sys.argv[1:])
Copy link
Contributor

Choose a reason for hiding this comment

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

new line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@yalcinmelihyasin yalcinmelihyasin merged commit 4acec0a into google:master Jul 4, 2022
@yalcinmelihyasin yalcinmelihyasin deleted the linter_integration branch July 5, 2022 11:18
rosasco-wk pushed a commit to rosasco-wk/agi that referenced this pull request Sep 2, 2022
Add flake8, autopep8 and mypy to our build pipeline and fix the errors from new linters
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.

3 participants