-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
[airbyte-ci] Format using a poe task #38043
Conversation
This commit moves the choice of formatting implementation into the pyproject.toml file. The benefit of this is that there's a guarantee that you can run `poetry run poe format` and `airbyte-ci format fix python` and they will do the same thing*. Except they will not.
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
@@ -30,22 +39,6 @@ omit = [ | |||
"**/generated/*", | |||
] | |||
|
|||
[tool.flake8] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meh, let's nuke this in favor of Ruff (soon). Happy to extract into a separate one, but thought I'd remove it while I was there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly putting this back solves the formatting mismatch in CDK, so perhaps I should scope this PR only on formatting, and not linting, and that might be easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working on it, one sec.
@@ -30,22 +39,6 @@ omit = [ | |||
"**/generated/*", | |||
] | |||
|
|||
[tool.flake8] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly putting this back solves the formatting mismatch in CDK, so perhaps I should scope this PR only on formatting, and not linting, and that might be easier.
"**/generated/*", | ||
] | ||
max-complexity = 20 | ||
max-line-length = 140 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically removing this line results in a BARRAGE of problems in python CDK because it tries to do flake check.
There are two paths forward — either remove the flake check before this PR, or remove it from this PR and do after.
Ok, this should be pretty small and easy change now, marking as automerge and asking for @aaronsteers and @alafanechere quick review. I will not push forward on
|
1 similar comment
Ok, this should be pretty small and easy change now, marking as automerge and asking for @aaronsteers and @alafanechere quick review. I will not push forward on
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to my eyes. 🚀
Others might have more context on historical exclusions.
Also - thanks for the detailed walkthrough in the PR description. Makes sense 👍
What
This PR moves the choice of formatting implementation into the pyproject.toml file. I'm taking the Ruff migration slow now, and want to chunk it up into very small pieces, and understand how our tools work. So, bear with me! I have a story for you. /cc @alafanechere and @aaronsteers.
Why
The benefit of this small step is that there's a guarantee that you can run
poetry run poe format
andairbyte-ci format fix python
and they will do the same thing*. Except they will not.What did I learn today?
Earlier I was running around saying that "airbyte-ci format only runs on changed files" — I was wrong, as @alafanechere pointed out. But I couldn't let it go — I remembered that when I run
black .
on our repo, I have a lot of changed files.Surely they would be flagged by
airbyte-ci format check
if it ran on all files, right? I believe it would, BUTairbyte-ci format check
does NOT run on ALL files — it runs on most files. See this list of ignored files.My working theory is that it's sane to exclude them, and airbyte-ci does, but
black .
does not.What's a good formatting and linting experience
With modern formatting and linting and type checking tools for Python, I would love my team to have experience where:
For that to be true, we need a formatter configuration that can be run VERY quickly locally, but be consistent with CI. Meaning, we need a way to have the same ignorelist in
black
itself.Okay, no problem, we can use
--exclude
or exclude setting inpyproject.toml
and remove the corresponding items from the exclusions list in the code that I pointed out above.But wait, we should clean out that exclusions list!
Just as I was writing this up, I found https://github.com/airbytehq/airbyte-internal-issues/issues/2773. I don't see anything that would stop us from applying formatting to
__init__.py
files. There are two more excluded patterns:**/tools/git_hooks/tests/test_spec_linter.py
does not exist anymore.**The goal of this PR is to nail down the exclusion list, so no additional formatting is required, aside from
__init__.py
files.Can this PR be safely reverted and rolled back?