Skip to content

Conversation

evgeni
Copy link
Member

@evgeni evgeni commented Jun 17, 2025

sometimes filling in a template leads to black changes (when an URL makes a line too long, for example), so lets fix that directly when generating, avoiding a second "black" commit

@evgeni
Copy link
Member Author

evgeni commented Jun 17, 2025

This is especially annoying when the resulting diff (update ci + black) is +0-0 because the only changes that happened are "regenerate a .py file, making a line non-black compliant" followed by "black".

plugin-template Outdated
Comment on lines 413 to 414
if os.path.exists(destination):
subprocess.run(["black", "--quiet", destination])
Copy link
Member

Choose a reason for hiding this comment

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

I like the idea. Is it evaluating settings in pyproject.toml here?

Copy link
Member Author

Choose a reason for hiding this comment

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

As pwd is the template checkout, not the plugin checkout, probably not. Should I try to make it to?

Copy link
Member

Choose a reason for hiding this comment

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

In case of the bootstrap, we might not yet have it.
So better to make sure we get the same config somewhere else.

Copy link
Member

Choose a reason for hiding this comment

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

Or just run black on everything after all the templates.

Copy link
Member Author

Choose a reason for hiding this comment

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

How? :)
But in the case of bootstrap, it will be fixed in a single "black" commit at some point and everything is good (again).

Updated the code to use cwd=plugin_root, so it should pickup the settings (untested)

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to avoid blacking files that do not come from a template, so that the "update CI files" commit is really that: CI files, not "ooh look, new black rule, let's refactor half of your repo"

Copy link
Member

Choose a reason for hiding this comment

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

Well, someone/thing needs to do it...

Copy link
Member

Choose a reason for hiding this comment

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

Maybe after all that was the reason to have it in a separate commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

That (generic) thing is

if [[ "${use_black}" == "True" ]]
then
pip install -r lint_requirements.txt
black .
if [[ "$(git status --porcelain)" ]]
then
git add -A
git commit -m "Reformat with black"
else
echo "No formatting change needed"
fi
fi

Copy link
Member Author

Choose a reason for hiding this comment

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

Hah, but posting this, I realized, black might not (yet) be installed when plugin-template runs

sometimes filling in a template leads to black changes (when an URL
makes a line too long, for example), so lets fix that directly when
generating, avoiding a second "black" commit
@evgeni
Copy link
Member Author

evgeni commented Jun 17, 2025

Mhh, that... broke lint :D

@evgeni
Copy link
Member Author

evgeni commented Jun 17, 2025

I'm reasonably happy with the result.

Copy link
Member

@mdellweg mdellweg left a comment

Choose a reason for hiding this comment

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

I believe i convinced myself, that this is what we want. ;)

@mdellweg mdellweg merged commit 12a4af3 into pulp:main Jun 17, 2025
11 checks passed
@evgeni evgeni deleted the superblack branch June 17, 2025 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants