pre-commit: Add hooks for qmllint and qmlformat #3915
Conversation
|
What shall I say, that is why nix is superiour and does not require such hacks.... |
We cannot force our contributors to use a particular OS (= one with a non-outdated Qt version), or set up nix or docker, or build Qt5 themselves locally. I have no problems on my machine, as I use Qt 5.15 for quite some time from my distros official repos. The whole "hack" has only been added for people on outdated distros. |
Please stay on topic. We don't need to fuel the Linux distro wars. Let's just work together to keep Mixxx platform-independent. Thank you. |
|
Shall we add the corresponding Dockerfile to our repo, e.g. in |
nix is not a linux distribution, nix is a package manager that does things correct, is not invasive and actually collects it's garbage. I made a technical comment because I think this is wrong. Now we have to maintain even more distribution specific stuff, regullarly build docker containers, push those etc, who takes care about that, are those containers CI built ? |
We don't have to rebuild them unless we want to use a more recent Qt/clang-format version. It's fine that you're passionate about nix, but do you really suggest to required setting up nix in order to contribute to Mixxx? Because I don't see another way to get around this if we don't want this "hack". |
Holzhaus
force-pushed
the
qmlformat-pre-commit
branch
from
1758ec2
to
4dcf6cb
Compare
|
Comments added. |
| types: [text] | ||
| files: ^.*\.qml$ | ||
| # Not enabled in commit stage by default, because qmlformat requires Qt | ||
| # 5.15 to be installed |
There was a problem hiding this comment.
Can't we just add a wrapper script that calls qmlformat if it exists or print a message "qmlformat is not installed, skipping qmlformat".
I will never remember running this with manual arguments
There was a problem hiding this comment.
You will notice when it runs on CI at least. I thought about that as well, but thr problem is that the hook will always pass in that case, and you don't really know it it actually passed or if the tool was just not found.
There was a problem hiding this comment.
This adds the qmllint and qmlformat hooks to ensure our QML files are up to par.
Unfortunately,
qmlformatis only part of Qt 5.15 and later. Therefore, we cannot expect our contributors to have it installed. Since it's a binary dependencies (i.e. not a python/npm package), we also can't provide convenient automatic installation.Therefore, the qmlformat hook is restricted to the
manualhook stage. This means that it's not run automatically when committing, only when you run:Not great and easy to forget, but we can let our pre-commit CI workflow assist us in noticing issues during PR review. However, this requires that our CI has Qt 5.15 available, which isn't the case with the current
ubuntu-latestimage.Hence, I wrote a small Dockerfile based on Arch Linux and uploaded the image to DockerHub, so that we can use it in our pre-commit workflow.
TL;DR: With this PR, QML formatting issues can be detected early on CI without requiring all contributors to install Qt 5.15 or later.