black-pr.yml and black.yml will be difficult to maintain

[eulertour]

The prior configuration for the formatting check, while less efficient, had a subtle yet significant benefit that it can be understood just as quickly as it can be read. The new ones, by contrast, are much more difficult to understand, debug, and modify [1][2]. And while additional features will inevitably lead to greater complexity, the tradeoff in this case seems to be more of a disadvantage in the long run.

Refactoring the current configuration to abstract away some of the string parsing and split up the code would greatly improve its maintainability, and in general it'd be best to keep code clarity in mind when reviewing PRs.

Tagging @leotrs @naveen521kk @huguesdevimeux for their work on the change.

[eulertour]

The more I look at these files the more they don't make sense to me.

For black-pr.yml:

• Why doesn't it checkout the repository with the checkout action provided by GitHub?
• Why is it beneficial to only format the files that were edited?
• Why run both black --diff and black on each file?
• Why manually log the edited files when black logs edited files by default?

For black.yml:

• Why remove newlines separating steps in the most densely packed part of the code?
• Why set fetch-depth to 0 during the checkout?
• Why manually check for modified files when black --check already does this?
• Why manually log the version of black when it's logged when it's installed?
• Have you considered using an action to create the pull request, like this one?

It's very important that we maintain code hygiene in order to develop efficiently, and right now these changes are making that goal more difficult to achieve.

[naveen521kk]

Reference #362 , #416 , #409

Why doesn't it checkout the repository with the checkout action provided by GitHub?

Yeah, it can be done using the one Github provides yes.

Why is it beneficial to only format the files that were edited?

It is enough to check what has been edited rather than checking for whole library formatting. This would prevent it from failing when it shouldn't.

Why run both black --diff and black on each file?

black --diff is to show what has to be changed by black and black actually formats the file to further check whether there are any files that are changed.

Why manually log the edited files when black logs edited files by default?

That is just for showcase. People would find it difficult on seeing the diff but could be a lot better to just what files have changed directly moving to a section. Possibly we can make it add as a review comment.

Why remove newlines separating steps in the most densely packed part of the code?

That can be easily added though.

Why set fetch-depth to 0 during the checkout?

I think that can be removed may be a redundant line.

Why manually check for modified files when black --check already does this?

I don't understand this. If I am right that is for the comment in the PR. See this comment for example. #508 (comment)

Why manually log the version of black when it's logged when it's installed?

That was to prevent creating conflicting branches. That was written in the aim to run daily but now it is not doing so.

Have you considered using an action to create the pull request, like this one?

First I didn't know there was an action like that and I couldn't find one. If that is helpful, that can be used. But the present implementation is just calling the API and that's it.

[eulertour]

It is enough to check what has been edited rather than checking for whole library formatting. This would prevent it from failing when it shouldn't.

This wouldn't happen if all of the code is formatted, were there places where we merged unformatted code?

black --diff is to show what has to be changed by black and black actually formats the file to further check whether there are any files that are changed.

Why do you want to see the changes?

That is just for showcase.

The extra step here just isn't necessary. It clutters the configuration without providing any benefit.

I don't understand this. If I am right that is for the comment in the PR. See this comment for example.

I think that's fine.

That was to prevent creating conflicting branches. That was written in the aim to run daily but now it is not doing so.

Two things: using the date alone would still accomplish that, and "Print Black Version if Failed" is a misnomer since the step is actually committing changes.

First I didn't know there was an action like that and I couldn't find one.

The benefit in this case is that it would make the step much clearer with no real drawback.

[eulertour]

Is anyone opposed to me rewriting these?

[leotrs]

No objection. @naveen521kk ?

[naveen521kk]

Is anyone opposed to me rewriting these?

No problem.

Also, Alternative to the action is using GitHub CLI.

[naveen521kk]

@eulertour are you working on this?

[eulertour]

No, I got sidetracked with other things

[behackl]

black.yml and black-pr.yml have been replaced. The issue can be closed.