Skip to content

Use black for formatting#145

Merged
pehala merged 2 commits intoKuadrant:mainfrom
pehala:black
Feb 23, 2023
Merged

Use black for formatting#145
pehala merged 2 commits intoKuadrant:mainfrom
pehala:black

Conversation

@pehala
Copy link
Copy Markdown
Contributor

@pehala pehala commented Nov 9, 2022

This is what black looks like with --line-length set to 120

This is an experimental PR which showcases what black would look like and I would like to hear what you think about code quality.

For Pycharm users, see https://black.readthedocs.io/en/stable/integrations/editors.html for configuring support for black.

@pehala
Copy link
Copy Markdown
Contributor Author

pehala commented Dec 6, 2022

Rebased to the latest main, please let me all know what kind of changes you do not like so we can discuss them

Comment thread testsuite/tests/kuadrant/authorino/operator/tls/conftest.py Outdated
Comment thread testsuite/tests/kuadrant/authorino/operator/tls/conftest.py
Comment thread testsuite/certificates/__init__.py
Comment thread testsuite/httpx/auth.py
Comment thread testsuite/mockserver.py Outdated
urljoin(self.url, "/mockserver/expectation"),
verify=False,
timeout=5,
json={
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

JSON one-line formatting is arguably worse readable.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is an option to leave dictionary formatting in the "traditional way" if black sees a trailing comma after the last element.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That would be great, but black can't be configured if I remember correctly. Its rules are set (that is what is good about black). But we can still use ignore

Copy link
Copy Markdown
Contributor Author

@pehala pehala Jan 9, 2023

Choose a reason for hiding this comment

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

@averevki You are right! Thanks for the suggestions. https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#the-magic-trailing-comma. It is not a config option, but if you have a comma after the last element in dict/Func call/Tuple it won't get squashed to a oneliner. @jsmolar Could you show me all locations where you would like to get rid of oneliners?

jsmolar
jsmolar previously approved these changes Dec 7, 2022
@averevki
Copy link
Copy Markdown
Contributor

I like the overall consistency of the code. I believe long one-liners can be solved by setting a shorter line length (e.g. to black default 88 characters), if this is an option.

@jakurban
Copy link
Copy Markdown

jakurban commented Jan 9, 2023

As the saying goes "new year, new me", after the new year I somehow started to feel that black is a welcome change in our project hence I approved this PR.

jakurban
jakurban previously approved these changes Jan 9, 2023
@pehala pehala dismissed stale reviews from jakurban and jsmolar via 3702230 January 9, 2023 08:37
@pehala pehala force-pushed the black branch 2 times, most recently from 3702230 to 3d45538 Compare January 9, 2023 08:38
@pehala pehala marked this pull request as ready for review January 9, 2023 08:41
@pehala
Copy link
Copy Markdown
Contributor Author

pehala commented Jan 9, 2023

I like the overall consistency of the code. I believe long one-liners can be solved by setting a shorter line length (e.g. to black default 88 characters), if this is an option.

I consider a length of 120 to be still reasonable also thanks to your suggestion with the magic trailing comma we have an option to "disable" one-liners.

jsmolar
jsmolar previously approved these changes Jan 18, 2023
jakurban
jakurban previously approved these changes Jan 18, 2023
averevki
averevki previously approved these changes Jan 20, 2023
@pehala pehala dismissed stale reviews from jakurban and jsmolar via 744bff4 February 22, 2023 14:31
@pehala pehala merged commit 8c68e03 into Kuadrant:main Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants