helix icon indicating copy to clipboard operation
helix copied to clipboard

Add: validation of bundled themes in build workflow

Open sarsapar1lla opened this issue 1 year ago • 3 comments

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

sarsapar1lla avatar Sep 03 '24 19:09 sarsapar1lla

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

the-mikedavis avatar Sep 03 '24 21:09 the-mikedavis

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

sarsapar1lla avatar Sep 04 '24 20:09 sarsapar1lla

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

sarsapar1lla avatar Sep 04 '24 21:09 sarsapar1lla

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

sarsapar1lla avatar Sep 23 '24 19:09 sarsapar1lla