-
Notifications
You must be signed in to change notification settings - Fork 37
automatically run black on files we update via templates #965
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
Conversation
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
if os.path.exists(destination): | ||
subprocess.run(["black", "--quiet", destination]) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That (generic) thing is
plugin_template/scripts/update_ci.sh
Lines 43 to 55 in 26877f9
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 |
There was a problem hiding this comment.
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
Mhh, that... broke lint :D |
I'm reasonably happy with the result. |
There was a problem hiding this 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. ;)
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