InvokeAI
InvokeAI copied to clipboard
invoke-ai
Introduce pre-commit, black, isort, ...
This PR aims to make things easier for new contributors as well as sort out common problems before commiting
Feel free, no prob. Now that I know I will leave it untouched till you're done.
I was also already thinking about if instead of just having one document, make a Contributors Folder with different toppics, like the already begun "how to create a PR", but also pointing to possible topics for contributions, like the translations, GH-Actions, u name it, since this would maybe help to encourage / enable people who do not have a background in development but still would like to help out 😅
I think that's a good evolution. We can refactor the doc at some point if needed! I want to split up what you've written into a non-technical (about the project, philosophy, etc.) and technical and expand where I can.
ping @JPPhoto - will you commit the changes to this draft PR, or can I continue updating 010_HOW_TO_CONTRIBUTE.md?
@lstein Kind of yes - it will imho not be from value to merge this PR without enabling the pre-commit hooks, but enabling them comes with the price that black and isort would be enabled, which was decided should happen after the refactoring (at least this is the latest information I got regarding this topic)
Nice to finally hear from you, hope you are fine and will at some point come back to InvokeAI 🙈 Also thx for letting me and others know your opinion while leaving it up to them 👌🏻
isort seems like a good idea.
I object to black's formatting due to this kind of thing:
https://github.com/invoke-ai/InvokeAI/blob/11a29fdc4d3c7dcb9b6bfe56ea22e4122933701f/ldm/invoke/CLI.py#L995-L997
https://github.com/invoke-ai/InvokeAI/blob/11a29fdc4d3c7dcb9b6bfe56ea22e4122933701f/ldm/modules/textual_inversion_manager.py#L411-L413
Those are not good interpretations of any reasonable coding standard.
@keturn I did not run the hook here yet, so the formatting of the files is not black at all (unless the coder used it, but since it is only formatting complete files, not only your changes, this will not be the case).
When I re-format it manually to look like:
def choose_postprocess_name(opt, prefix, seed) -> str:
match = re.search("postprocess:(\w+)", opt.last_operation)
if match:
# will look like "gfpgan", "upscale", "outpaint" or "embiggen"
modifier = match.group(1)
else:
modifier = "postprocessed"
black would not touch the file at all 🙈
black ./ldm/invoke/CLI.py
All done! ✨ 🍰 ✨
1 file left unchanged.
The examples I quoted were from commits formatted with black. Reproducible with the playground.
But you saw that the code was already formated like this beforehand, right?
People would still have the chance to reformat their code before committing, when the pre-commit hooks kick in. And it is not forced to look bad with black as this reformated example can prove.
So imho, there is never a bulletproof version, unecesarry which code formatter we would apply, but we should still establish standards!
Standards are fine. PEP 8 is fine.
Automatically running every commit through black is not a good way to meet those standards.
@mauwii Would it be possible to rebase this against v2.3 ? I'd like to play with this on a slow-moving branch rather than on main where a lot of people will be working.
I tried rebasing by pressing the edit button above, but the PR ended up containing a bunch of nodes-related files that were carried over from merge commits into the PR (I think).
FTR:
I object to black's formatting due to this kind of thing
- Absolutely agree
- Warts like that are usually because the line length is too short (still a black problem, of course; their default is demonstrably ridiculous). If you force black to use longer lines, things like that are much less common.
