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

Enable black formatter #49

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Enable black formatter #49

wants to merge 2 commits into from

Conversation

gthvn1
Copy link
Contributor

@gthvn1 gthvn1 commented Oct 23, 2024

Black is a PEP 8 compliant opinionated formatter with its own style.

We want to use Black that is a PEP 8 compliant opinionated formatter.
This commit has been generated by running `black --unstable` on python
files. In the next commit we will add a workflow that will run this
automatically.

Signed-off-by: Guillaume <[email protected]>
@gthvn1 gthvn1 force-pushed the enable-black-format branch from b34acd7 to 0435244 Compare October 23, 2024 15:36
@gthvn1 gthvn1 marked this pull request as draft October 23, 2024 15:39
We don't format the code. It is up to the developer to call black on its
own code. The workflow only checks that formatting is ok.

Signed-off-by: Guillaume <[email protected]>
@gthvn1 gthvn1 force-pushed the enable-black-format branch from 5531ac5 to 8d532c0 Compare October 23, 2024 15:43
@gthvn1 gthvn1 requested a review from Wescoeur October 23, 2024 15:44
@gthvn1 gthvn1 marked this pull request as ready for review October 23, 2024 15:44
@Wescoeur Wescoeur requested a review from benjamreis October 23, 2024 16:35
@stormi
Copy link
Member

stormi commented Oct 23, 2024

What problem is this PR solving in the first place? The commit message says "We want to use Black that is a PEP 8 compliant opinionated formatter", but it's not enough of a reason, as black is not just something that helps you being compliant with PEP 8 (as pycodestyle does already), but also decides that there's only one right way to format, and this is the main change which is not motivated in the commit message.

While not entirely against, I have several concerns:

  • We use the same branch of this repo for XCP-ng 8.2 LTS, XCP-ng 8.3, and future development. Either we'll have to change this to maintain a 8.2 branch, or we'll have to push dozens of uneeded changes to a LTS.
  • We already have pycodestyle, which forces us to respect basic formatting rules but also leaves space for adaptation. On the other hand, black only accepts one version of how it should be formatted, which is a lot stricter than PEP 8.
  • Not everyone develops with autoformatters in an IDE. We have some who use console editors too. They will have to either remember to run black after a change, or see their PRs rejected. This is also something that can make it harder for external people to contribute. See how hard it is already to ask most first-time contributers to do simple changes in their PRs.

@stormi stormi requested review from stormi and ydirson and removed request for ydirson October 23, 2024 16:48
@gthvn1
Copy link
Contributor Author

gthvn1 commented Oct 23, 2024

The main reason is to have a better homogeneous code that is more readable IMHO. But I understand your remarks so we can ignore the PR no worries. Another annoying thing is that it breaks a little bit the search using "git blame" to understand some changes. I'm using it for bigger project and refactoring is always annoying for git search...

And about the setup of IDE, it is true that it's up to the developer to setup its environment but you can setup your IDE or setup some git hook in your local repo or run it manually and there are probably other ways to do it. It is not a big constraint IMHO.

@stormi
Copy link
Member

stormi commented Oct 23, 2024

It is not a big constraint IMHO.

I'm afraid it is for occasional contributors. It raises the bar for people who sometimes already struggle with git and github. We can always help them, but experience shows the more things we ask, the more some are prone to giving up.

Copy link
Contributor

@benjamreis benjamreis left a comment

Choose a reason for hiding this comment

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

I'm all for it.

@ydirson
Copy link

ydirson commented Oct 24, 2024

My experience with formatters is that they sometimes get in the way when something complicated has to be formatted specially to make it easier to understand. I have a general dislike for that kind of tools.

@Wescoeur
Copy link
Member

My experience with formatters is that they sometimes get in the way when something complicated has to be formatted specially to make it easier to understand. I have a general dislike for that kind of tools.

It's why there is a specific feature in many formatters to handle these cases: https://black.readthedocs.io/en/stable/usage_and_configuration/the_basics.html#ignoring-sections

@Wescoeur
Copy link
Member

I'm afraid it is for occasional contributors. It raises the bar for people who sometimes already struggle with git and github. We can always help them, but experience shows the more things we ask, the more some are prone to giving up.

I will try to be pragmatic:

  • How many contributions do we have on python projects, and more specifically this repo?
  • How likely is it that someone who is already struggling with git will have trouble to exec a format command manually?
  • Also someone who contributes on python probably knows how to use pip to install a dependency. In the worst case we can help.

@ydirson
Copy link

ydirson commented Oct 24, 2024

My experience with formatters is that they sometimes get in the way when something complicated has to be formatted specially to make it easier to understand. I have a general dislike for that kind of tools.

It's why there is a specific feature in many formatters to handle these cases: https://black.readthedocs.io/en/stable/usage_and_configuration/the_basics.html#ignoring-sections

I would have to use it much too often, and that defeats the whole purpose of the tool.

But this is a discussion we should have a project level, not on any particular project like this.

@gthvn1 gthvn1 marked this pull request as draft October 24, 2024 09:24
@Wescoeur
Copy link
Member

I would have to use it much too often,

How can you be totally sure about that?

But this is a discussion we should have a project level, not on any particular project like this.

I completely agree. But complicated to set up currently on the installer, and even less on SMAPIv1. But for v3 it is clearly up for discussion in my opinion.

@gthvn1
Copy link
Contributor Author

gthvn1 commented Oct 24, 2024

My experience with formatters is that they sometimes get in the way when something complicated has to be formatted specially to make it easier to understand. I have a general dislike for that kind of tools.

It's why there is a specific feature in many formatters to handle these cases: https://black.readthedocs.io/en/stable/usage_and_configuration/the_basics.html#ignoring-sections

I would have to use it much too often, and that defeats the whole purpose of the tool.

Sometimes when the code is to complicated to be formatted means that code needs to be rewritten. I don't say it is always the case and I'm sure that we will find cases where disabling formatting is justified. But when there are issues with formatting it is often a good time to think differently ;-)

@ydirson
Copy link

ydirson commented Oct 24, 2024

How can you be totally sure about that?

By simply running it on a single file I wrote recently

@ydirson ydirson self-requested a review October 24, 2024 10:21
Copy link

@ydirson ydirson left a comment

Choose a reason for hiding this comment

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

Must be discussed at project level first

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