-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
pre-commit: Use qmlformat by default #3923
Conversation
This is no longer necessary thanks to the previous commit.
258af27
to
afea7ce
Compare
I agree. The number of developers that are affected will decline over time. |
qmlformat_executable = shutil.which("qmlformat") | ||
if not qmlformat_executable: | ||
print(QMLFORMAT_MISSING_MESSAGE.strip(), file=sys.stderr) | ||
return 1 |
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.
Here we return exit code 1 so that the user is prompted to either install qmlformat or skip the hook manually.
We could also return exit code 0. Thdi would be even more convenient, but then the hook shows up as "passed" and the user has no way of knowing if qmlformat actually ran or not. This is an issue, especially on CI. Therefore, I prefer the "skip manually" approach.
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 can imagine that it can leads to extra hassle when resolving conflicts, because it does not run on diffs only.
So I would prefer exit code 0 version, since it will be visible in our CI anyway.
That follows the approach that the whole pre-commit is optional anyway. Compared to that. this is one only a side issue, where we should not annoy a contributor that has just decided to install pre-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.
So I would prefer exit code 0 version, since it will be visible in our CI anyway.
We cannot be sure about that. It will display as passed
on CI in any case, even if your CI image does not actually have qmlformat.
I think the extra hassle is manageable. Just run this once:
$ echo SKIP=qmlformat >> ~/.profile
And don't forget remove it after upgrading to Qt 5.15 ;-).
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.
Yes ok. Ready to merge IMHO.
Just an idea, considering that we should not forget this .profile value.
How about use:
qmake --version
To exit 0 if qmlformat cannot be installed.
@daschuer Merge? |
Yes. |
@poelzi pointed out that it's easy to forget to use the manual hook stage to run
qmlformat
manually, and I agree.The hook will only run if you changed QML files anyway. Therefore, I think we can just enable the hook by default. If
qmlformat
is not installed, the user may skip the hook manually.For contributors without experience with
pre-commit
, I added a wrapper script that displays an error description and a way to work around it ifqmlformat
is missing: