helix
helix copied to clipboard
Add: validation of bundled themes in build workflow
Added xtask to validate bundled themes in order to detect issues at build time. Basically just loading all the themes are surfacing any warning logs which are emitted.
Main issue with the current implementation in the CI workflow is that it will cause builds on master to fail until the theme validation issues are addressed by their respective maintainers. Alternatively we could add a separate GitHub workflow which only triggers when the runtime/themes directory is edited but this felt like overkill in this instance.
Add: xtask for validating bundled themes Update: build workflow to run theme validation on build
Closes #5709
For the themes that currently fail to pass this we can comment out the offending keys and link to the issue. They're basically discarded while loading so commenting them out should be equivalent
Updated the implementation to explicitly return validation failures rather than use log captures. This involved adding a new public factory constructor on the Theme struct, Theme::from_keys, which allows for the creation of a theme from a toml map. This felt less intrusive than altering the return type of Loader.load plus allows the From<Value> and Deserialize impls for Theme to still work.
Please let me know if I've misunderstood what you were suggesting!
EDIT: I see now that we need to update the loader in order to correctly handle inherited styles and palette colours. Will update
I've updated the implementation to return both the theme and any validation failures from the Loader.load method and logged the failures at each call site. I've also updated the affected themes and commented out the invalid keys as per your suggestion.
The theme_check task now completes successfully and the warnings no longer appear in the logs when selecting themes such as emacs.
Let me know what you think @the-mikedavis
Introduced helper method as suggested. Now that the warnings are being logged in the same place, I updated the warning messages to include the theme name to make it clearer to users which theme is the source of the warning