Repository navigation
Align lint config with upstream ESPHome; validate and compile in CI - #88
Merged
Merged
Conversation
Comparing this repo's tooling against esphome/esphome turned up one gap that mattered more than the rest: ruff was installed but never configured. With no pyproject.toml it fell back to its defaults (E4, E7, E9, F), which is roughly pyflakes. Upstream selects ~30 rule families. Most visibly, isort was off, so import ordering was never enforced despite ruff-format running on every commit -- formatting and import sorting are separate concerns in ruff. The CI side had a different problem. Nothing validated the device configs at all. yamllint checks whitespace; it cannot catch an undefined substitution, a broken package reference, or a component option renamed by an ESPHome upgrade. For a repo that is mostly device configs, that was the largest real risk, and nothing verified the custom C++ components still compiled either. Changes: - Add pyproject.toml mirroring upstream's [tool.ruff] and [tool.pylint]. One deliberate divergence: upstream sets known-first-party = ["esphome"] because it is esphome; here esphome is a dependency, so it belongs in the third-party section and this repo's code is reached by relative imports. - Unify the Python version on 3.12 (CI, pyupgrade, ruff target-version). It was previously three-way inconsistent: CI on 3.11, pyupgrade on py311-plus, and ruff defaulting to py39 with no requires-python. - Add pylint, with esphome as a hook dependency so the component imports resolve. esphome.components.econet is in ignored-modules because devices pull it from esphome-econet at config time; it can never resolve statically. - Widen the flake8 scope to cover scripts/ as well as components/. - Fix what the new rules found: import ordering (ruff --fix), one nested if (SIM102), one range(len(...)) loop (C0200). CI: - Merge lint.yml and the new config/compile jobs into a single ci.yml, so the triggers and the concurrency group are defined once. Check contexts are job names, so this rename does not affect required-check config. - Skip no-commit-to-branch in CI. It is a local guardrail against committing to master, and it failed every post-merge push to master -- the red X after each merge was always this and nothing else. - Validate every device with esphome config. devices/secrets.yaml is gitignored, so scripts/generate_ci_secrets.py writes a placeholder one from the !secret references found in tracked YAML. - Compile five devices covering all six custom components across both platforms. The components compile behind USE_ESP32/USE_ESP8266 guards, so an esp32 build says nothing about the esp8266 ones. Compiles are skipped on feature-branch pushes and run on PRs and master. Remove .deepsource.toml. Its Python analyzer had been failing master on 14 issues that this repo's own config deliberately ignores -- an 88-character line limit against the configured 120, and missing docstrings under D100-D107. Its C & C++ analyzer reported 0 issues across 3708 lines, which reflects having no build configuration to resolve the esphome or ESP-IDF headers rather than clean code. The compile matrix has not run yet; this PR is its first exercise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ruff's EXE001 flags a shebang on a non-executable file. Both scripts are invoked as `python scripts/...`, so the shebangs were decorative, but marking the files executable matches upstream's script/ convention and keeps the shebang meaningful. The rule is skipped on Windows, where there is no executable bit, so this only ever fails in CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pushing a feature branch with an open PR ran every job twice, once for the
push event and once for the pull_request event, and the push run published
a check for the skipped compile job under its unsubstituted matrix name
(`compile ${{ matrix.device }}`), which is noise in the checks list and a
hazard if status checks are ever made required.
Branches are still fully covered from the moment their PR is opened, and
master still gets a post-merge run. The trade-off is that a branch pushed
without a PR now gets no CI at all.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nuttytree
added a commit
that referenced
this pull request
Aug 5, 2026
The compile jobs added in #88 prove the custom components still build, but a build only catches what the compiler rejects. Upstream ESPHome runs clang-tidy over every C++ source with a large check list; nothing here did, so the components drifted from the conventions and analysis ESPHome core is held to. Adopting it is not just a matter of pointing clang-tidy at components/. clang cannot consume the xtensa/riscv GCC command line the real build uses, and the components only compile against a device's generated esphome/core/defines.h. Upstream solves both with dedicated "tidy" PlatformIO environments; this repo has no platformio.ini of its own, so the flags come from an actual device build via `esphome idedata` -- which the CI compile matrix already produces. Changes: - Add .clang-tidy, a verbatim copy of upstream's, so a component here is held to exactly the checks it would face in ESPHome core. - Add scripts/clang_tidy.py, a slimmed-down port of upstream's script/clang-tidy: it reads the compiled build's idedata, rebuilds the clang command line from it (dropping GCC-only flags, macro-replacing the Arduino/newlib headers that use GNU extensions), and checks only the components that device built, in parallel. - Run it in the existing compile matrix, right after each `esphome compile`, so it costs no extra build. A component that runs on both platforms is analyzed once per platform, same as the compile itself. Register a problem matcher so findings annotate the diff. - Two adaptations upstream does not need. It analyzes at ESPHOME_LOG_LEVEL_VERY_VERBOSE, because these devices build with `log_level: none`, which preprocesses every ESP_LOG* call away -- hiding the code inside them and making log-only variables look unused. And it passes -fgnuc-version=4.2.1, which is what clang already assumes on Linux; on Windows it defaults to an MSVC target that leaves __GNUC__ undefined, and newlib's __GNUC_PREREQ ladders then fall back to `throw()` exception specifications, a hard error under C++17. CI is unaffected either way. The new checks found real problems in two components, fixed here so CI lands green: - pool_controller: reset_all_pump_runtimes_() logged this->last_check_->hour with no has_value() guard at all; it now takes the boundary time as a parameter, which also makes it explicit that the value is the walked time while catching up, not the current clock. The catch-up loop's own accesses are unprovable across calls clang-tidy cannot see into, so it binds the contained value once under the guard. - econet_zone_control: update_zone_fan_mode_() dereferenced min_zone/max_zone unconditionally after the scan, which is null if no zones are configured. The schema requires at least two, so this is unreachable today; guarded rather than left to depend on a validator elsewhere. - pool_controller: eight protected methods in pump_switch.h now carry the trailing underscore ESPHome uses for non-public members, which the file already used for others. Renamed by hand: clang-tidy's --fix renamed has_current_sensor() inside `#ifdef USE_SENSOR` and left the `#else` definition on the old name, which builds here and breaks a device without sensors. - Concatenated nested namespaces across pool_controller, and added braces to two multi-line statement bodies. - The one finding not fixed is a bugprone-branch-clone in update_current_action_(): "Off" is tested first and deliberately shares the fallback's body so it wins over a state string containing another keyword. Suppressed with a NOLINT and the reason, since the datapoint's value set is not documented anywhere. Verified by compiling all five devices in the CI matrix from scratch copies and running the new script against each: every component passes. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
Brings this repo's code quality checks in line with esphome/esphome, and adds the CI that was missing entirely: nothing validated the device configs, and nothing verified the custom C++ components still compiled.
Lint
pyproject.tomlit fell back to its defaults (E4,E7,E9,F) while upstream selects ~30 rule families. Most visibly isort was off, so import ordering was never enforced despiteruff-formatrunning on every commit. Newpyproject.tomlmirrors upstream's[tool.ruff]and[tool.pylint].py311-plus, ruff defaulting to py39. Now 3.12 everywhere.esphomeas a hook dependency so component imports resolve.scripts/.ruff --fix), oneSIM102, oneC0200.CI
lint.ymland the new jobs into a singleci.yml. Check contexts are job names, so the rename doesn't affect required-check config.no-commit-to-branchin CI. It's a local guardrail against committing to master, and it failed every post-merge push to master — the red X after each merge was always this and nothing else.esphome config.devices/secrets.yamlis gitignored, soscripts/generate_ci_secrets.pywrites a placeholder one from the!secretreferences in tracked YAML.USE_ESP32/USE_ESP8266guards, so an esp32 build says nothing about the esp8266 ones. Skipped on feature-branch pushes; runs on PRs and master.Removed
.deepsource.tomlIts Python analyzer had been failing master on 14 issues this repo's own config deliberately ignores — an 88-char line limit against the configured 120, and missing docstrings under
D100-D107. Its C & C++ analyzer reported 0 issues across 3708 lines, which reflects having no build configuration to resolve the esphome/ESP-IDF headers rather than clean code.Verification
pre-commit run --all-filespasses on all 11 hooks.esphome configpasses locally on every device whose components were touched (pool,pool-lights,master-bed,water-heater,hvac).🤖 Generated with Claude Code