ci: lint QML files with qmllint - #10855
Open
Alwoch wants to merge 2 commits into
Open
Conversation
Alwoch
marked this pull request as draft
August 14, 2026 14:39
Alwoch
marked this pull request as ready for review
August 17, 2026 09:08
Member
|
@Alwoch thank you for this, it looks quite comprehensive. Maybe we could for now mark the job/step as |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #10802
This PR creates a
qml-lintCI job that runs qmllint over all tracked*.qmlfiles (qml gui and plugins). The project currently usesPyQt6which doesn't shipqmllint, so in this PRqmllintis taken from the PySide6 wheels pinned in the workflow.PySide6-Addonsis also needed for the QtMultimedia types used by QRScan.qml. It is a lint-only dependency so nothing in electrum imports it.As anticipated in the issue, the default behaviour is not usable as a gate because a plain
qmllintover the tree currently emits approximately 3400 warnings, nearly all from three structural sources that static analysis cannot see: context properties injected at runtime (qeapp.py), dynamically scoped root-window properties (constants/appin main.qml), and theorg.electrumtypes being registered from Python. To address this:contrib/qml_lint/run_qml_lint.pygenerates a minimal qmldir+qmltypes stub fororg.electrumfrom theqmlRegisterTypecalls in qeapp.py (regenerated on every run, so it cannot drift) and passes it to qmllint via-I. With the module resolvable, a misspelled type name fails the lint instead of producing unresolved-import noise..qmllint.iniat the repo root disables six warning categories; each carries a comment with the measured warning count and what it would take to enable it. Everything else gates: syntax errors, unknown types, bad imports, and the 56 remaining default-enabled categories the tree already passes (duplicate bindings, writes to read-only properties, use-before-declaration, deprecations, ...).Known gaps:
Property-name typos and misspelled identifiers inside bindings are not caught yet because they live in the disabled MissingProperty/UnqualifiedAccess categories. The disabled categories are intended as a ratchet, not a permanent state; this PR is deliberately infra-only, and I plan to follow up in roughly this order:
Quick.LayoutsPositioningflags (raw width/height on items managed by a Layout, e.g. About.qml:87 -- the file the issue quotes), then enable the category.PropertyChangessites, then enableQuick.PropertyChangesParsed.MissingProperty(catching property-name typos) andUnresolvedAlias(1 site).UnqualifiedAccess(~3000 warnings today: ~800constants, ~790 context properties, ~260app, rest delegate scope). Unlike the items above, this changes runtime code, so it needs discussion first.The 96 unused-import notices the job prints are also a candidate for a small cleanup PR; they are info-level and do not gate. The one remaining disable,
EqualityTypeCoercion, is intentional and permanent: its single hit relies on url/string coercion, and switching it to!==would change behavior.Defaults I picked that are easy to change:
The linter is pinned to 6.11.1 (newest) while the android gui ships Qt 6.10.2. I can test and repin to 6.10.x if matching the runtime Qt is preferred.
.qmllint.inisits at the repo root so it also covers the plugin qml files; it could instead live under electrum/gui/qml/, at the cost of not covering plugins. And plugin qml files are in scope, since they currently pass clean.Let me know if this is along the lines of what you had in mind @SomberNight .