Skip to content
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

Merged
merged 2 commits into from
May 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 1 addition & 9 deletions .github/workflows/pre-commit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,24 +25,16 @@ jobs:
# on Pull Requests!).
env:
SKIP: end-of-file-fixer,trailing-whitespace,clang-format,eslint,no-commit-to-branch
with:
# Enable extra hooks that require special system dependencies that we
# cannot expect to be installed on all contributor's systems.
extra_args: --hook-stage manual --all-files

- name: "Detect code style issues (pull_request)"
uses: pre-commit/action@v2.0.0
if: github.event_name == 'pull_request'
env:
SKIP: no-commit-to-branch
with:
# Enable extra hooks that require special system dependencies that are
# available in our custom docker container, but cannot be expected to
# be installed on all contributor's systems.
#
# HEAD is the not yet integrated PR merge commit +refs/pull/xxxx/merge
# HEAD^1 is the PR target branch and HEAD^2 is the HEAD of the source branch
extra_args: --hook-stage manual --from-ref HEAD^1 --to-ref HEAD
extra_args: --from-ref HEAD^1 --to-ref HEAD

- name: "Generate patch file"
if: failure()
Expand Down
7 changes: 1 addition & 6 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -126,16 +126,11 @@ repos:
- manual
- id: qmlformat
name: qmlformat
entry: qmlformat -i
entry: tools/qmlformat.py
pass_filenames: true
require_serial: true
language: system
types: [text]
files: ^.*\.qml$
# Not enabled in commit stage by default, because qmlformat requires Qt
# 5.15 to be installed
stages:
- manual
- id: qmllint
name: qmllint
entry: qmllint
Expand Down
37 changes: 37 additions & 0 deletions tools/qmlformat.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
#!/usr/bin/env python3
"""
Small qmlformat wrapper that warns the user if the tool is not installed.
"""
import argparse
import shutil
import subprocess
import pathlib
import sys

QMLFORMAT_MISSING_MESSAGE = """
qmlformat is not installed. It is included in Qt 5.15 and later. If that Qt
version is not available on your system, please use the SKIP environment
variable when committing:

$ SKIP=qmlformat git commit
"""


def main(argv=None):
qmlformat_executable = shutil.which("qmlformat")
if not qmlformat_executable:
print(QMLFORMAT_MISSING_MESSAGE.strip(), file=sys.stderr)
return 1
Copy link
Member Author

@Holzhaus Holzhaus May 29, 2021

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.

Copy link
Member

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.

Copy link
Member Author

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 ;-).

Copy link
Member

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.


parser = argparse.ArgumentParser()
parser.add_argument("file", nargs="+", type=pathlib.Path)
args = parser.parse_args(argv)

for filename in args.file:
subprocess.call((qmlformat_executable, "-i", filename))

return 0


if __name__ == "__main__":
sys.exit(main())