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

Add black formatter #3531

Merged
merged 8 commits into from Aug 14, 2021
Merged

Add black formatter #3531

merged 8 commits into from Aug 14, 2021

Conversation

Cjkjvfnby
Copy link
Contributor

@Cjkjvfnby Cjkjvfnby commented Jul 25, 2021

Add black formatter and reformat some files.

I use black for my personal projects, on my previous job, and when working with students. I am really happy with it.

Not the best style, but zero effort for support and config it.

This is the first part, I just added all required configs and reformated all python files outside the default diectory.

@Cjkjvfnby Cjkjvfnby added category:feature The Issue/PR describes or implements a new game feature. component:infrastructure The Issue/PR deals with development aiding infrastructure like CI, distribution or helper tools. labels Jul 25, 2021
@geoffthemedio
Copy link
Member

looks like it's all Python changes. what review do you want?

@Cjkjvfnby
Copy link
Contributor Author

looks like it's all Python changes. what review do you want?

Adding formatter is a project-level change. I want to be sure that you are aware of it.

name="SPY_CATEGORY",
graphic="spy.png",
colour=(168, 0, 255, 255))
Category(name="LEARNING_CATEGORY", graphic="learning.png", colour=(54, 202, 229))
Copy link
Member

Choose a reason for hiding this comment

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

I think this was more readable before the reformat

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line fits in one line (120) so it is inlined.

This is the disadvantage of such tools. They are better only for 90% of cases.

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 went to docs to answer other questions and found that this could be fixed.

https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#the-magic-trailing-comma

I will fix it tomorrow.

@@ -5,7 +5,8 @@
type=float,
default=1.0,
min=0.1,
max=10.0)
max=10.0,
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised this (, with nothing after before the )) isn't a format error in Python...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trailing comma is ok for function calls and list literals. For tuples with a single element, it is mandatory.

(1) == 1  # True
(1, ) == (1, )  # True

I find this feature very useful, since it reduce diff when adding new argument/element.

Copy link
Member

Choose a reason for hiding this comment

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

it reduce diff when adding new argument/element.

You mean just when the params are on separate lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On separate lines with coma:

- a
+ a,
+ b
+ b,

@@ -141,18 +147,17 @@ def update_stdafx_h(stdafx_h_filename, header_only_vcxproj_files, vcxproj_files)

lines.append(include_directive)

with open(stdafx_h_filename, 'w') as f:
with open(stdafx_h_filename, "w") as f:
Copy link
Member

Choose a reason for hiding this comment

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

why is " preferred over ' ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Cjkjvfnby
Copy link
Contributor Author

@geoffthemedio, I think I have addressed all your comments. I plan to merge it at a weekend.

@Cjkjvfnby Cjkjvfnby merged commit 7b8bcda into freeorion:master Aug 14, 2021
@Cjkjvfnby Cjkjvfnby deleted the paint-it-black branch August 14, 2021 06:24
@geoffthemedio geoffthemedio added the status:merged All relevant commits of this PR were merged into the master development branch. label Aug 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:feature The Issue/PR describes or implements a new game feature. component:infrastructure The Issue/PR deals with development aiding infrastructure like CI, distribution or helper tools. status:merged All relevant commits of this PR were merged into the master development branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants