Skip to content

ci: verify generated en.json is current and every entity has a name - #449

Closed
pos-ei-don wants to merge 1 commit into
tomer-w:mainfrom
pos-ei-don:feat-ci-translations-check
Closed

ci: verify generated en.json is current and every entity has a name#449
pos-ei-don wants to merge 1 commit into
tomer-w:mainfrom
pos-ei-don:feat-ci-translations-check

Conversation

@pos-ei-don

Copy link
Copy Markdown
Contributor

Companion to the library-side check in tomer-w/victron_mqtt, addressing the translations half of #104.

Adds a Generated Files Check workflow (push to main + PRs):

  1. Stale en.json — regenerates via .github/scripts/merge_topics.py and fails on any diff. merge_topics otherwise only runs in the version-bump workflow, so a manual catalog/translation edit could drift unnoticed.
  2. Missing display name — fails if any generated entity entry has a null/empty name (would render as device-name-only). Entries without a name key (main-device topics) are intentionally skipped.

Both pass on current main (en.json regenerates with no diff, 0 nameless entities). Pure stdlib + Python 3.14, matching validate.yaml.

If it ever gets in your way: disable it under Actions → Generated Files Check → ⋯ → Disable workflow, or remove it by deleting the workflow file. It isn't a required check, so a red run won't block merges unless you add it to branch protection.

Adds a 'Generated Files Check' workflow (push to main + PRs):

1. Regenerates the English translations via merge_topics.py and fails if
   the committed en.json differs - merge_topics otherwise only runs in the
   version-bump workflow, so a manual catalog/translation edit could drift.
2. Fails if any generated entity entry has a null/empty name, i.e. an
   entity that would render as the device name only.

Both pass on current main (en.json regenerates with no diff, 0 nameless
entities). Pairs with the library-side check in tomer-w/victron_mqtt.
@tomer-w

tomer-w commented Jul 1, 2026

Copy link
Copy Markdown
Owner

I'm not exactly sure what this PR is solving. The merge topics script is running when I run the workflow which takes new library version. This is exactly when we need it.
Empty name is not something that ever happened AFAIK. and should be check in the library as it will be library bug.

@pos-ei-don

Copy link
Copy Markdown
Contributor Author

Hey @tomer-w, thanks for looking at this!

To address your points:

What this PR solves: In your review of lib PR #109 you noted that if there's a fix for entities with missing localization names falling back to device name, "it is probably in the integration side." This PR is exactly that — a lightweight CI guard living on the integration side, as you suggested.

Why not rely on the merge topics script alone: The merge script runs during library version bumps. But en.json can also drift through manual edits between version bumps, or a library bug could introduce a missing entity name that gets picked up silently. The check here ensures we catch that at the integration level before any release, regardless of how the drift occurred.

"Empty name never happened AFAIK": Agreed — and this PR is designed to keep it that way. It's a regression guard, not a fix for an existing bug.

"Should be checked in the library": The JSON-validity check for victron_mqtt.json already lives there (your lib PR #109). The integration-level check in this PR is for en.json specifically — ensuring every entity that the library exposes has a localization entry. That's integration data, so it fits here.

The CI step is a single Python call with no new dependencies, so the maintenance burden is minimal. Let me know if you'd like any adjustments!

@tomer-w

tomer-w commented Jul 7, 2026

Copy link
Copy Markdown
Owner

I would remove the "Every entity has a display name" task. I dont think this is really checking something useful. There could be many other errors like not having the entity there at all (not just missing name). Also I just dont want to start supporting python scripts which are not running easily locally as part of our test harness. Overall, I don't see major value with this specific check.

@pos-ei-don

Copy link
Copy Markdown
Contributor Author

Makes sense — closing this. The stale-file check is already covered by the merge_topics.py script running at library version bumps (exactly the right moment), and the name check guards against a problem that hasn't occurred in practice. The maintenance cost of a custom Python script in CI outweighs the value here.

@pos-ei-don pos-ei-don closed this Jul 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants