🧰 Modernize project tooling, CI, and documentation#2031
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 37 minutes and 47 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis pull request modernizes the project's tooling and infrastructure by replacing mypy with ty for type checking, updating CI/CD workflows with security hardening practices, migrating pyproject.toml to PEP 621 standard, removing SonarCloud in favor of OpenSSF Scorecard, and adding comprehensive test fixtures and test suites covering CLI and model deserialization behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Add zeroconf/zeroconf.asyncio to pylint ignored-modules since they are optional dependencies not installed in CI - Handle None case in mac property lookup instead of suppressing the type error with a ty: ignore comment Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The CLI module imports zeroconf which is an optional dependency not installed in CI. Exclude src/wled/cli/ from ty type checking in both CI and pre-commit, consistent with the existing coverage exclusion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add comprehensive tests for CLI commands and AsyncTyper (29 tests) - Install optional CLI dependencies (--extras cli) in linting, typing, and test CI workflows so zeroconf/typer are available - Remove coverage omit for src/wled/cli/* — CLI is now tested - Revert ty exclude workaround — proper fix is installing the deps Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rewrite CLI tests to use syrupy snapshots instead of string assertions, following the python-twentemilieu pattern: - Add stable_terminal fixture for deterministic Rich rendering - Snapshot all command outputs (info, effects, palettes, etc.) - Snapshot error handler panel output via capsys - Add CLI structure test via click command introspection - Use @pytest.mark.usefixtures for fixtures without return values Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rich table output includes intentional trailing spaces. Exclude snapshot files from the trailing-whitespace hook, matching the pattern used in python-twentemilieu. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR modernizes the repository’s development tooling and CI configuration, and significantly expands/updates the test suite (including new CLI/model coverage) to match the updated WLED client behavior.
Changes:
- Replace/expand tests (notably moving HTTP mocking to
aioresponses) and add extensive new coverage for WLED client, models, and CLI output (snapshots + fixtures). - Modernize tooling/quality gates: move from mypy →
ty, introduceprek, tighten coverage thresholds, update linting/YAML rules, and refresh GitHub Actions with pinned action digests plus supply-chain/security workflows. - Update docs/examples and repository metadata (README badges, security policy, community docs, release process).
Reviewed changes
Copilot reviewed 48 out of 50 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_wled.py | Major expansion of client behavior tests (HTTP + websocket + upgrade + releases). |
| tests/test_models.py | New unit + snapshot coverage for models/serialization/utils. |
| tests/test_cli.py | New CLI and AsyncTyper tests with snapshot verification. |
| tests/conftest.py | Adds shared fixture loaders and aioresponses helpers. |
| tests/snapshots/test_cli.ambr | Adds snapshot baselines for CLI output and structure. |
| tests/fixtures/wled.json | Adds base WLED API fixture used across tests. |
| tests/fixtures/presets.json | Adds presets fixture used across tests. |
| tests/fixtures/rgb.json | Adds real-world RGB fixture for snapshot/model parsing. |
| tests/fixtures/rgbw.json | Adds real-world RGBW fixture for snapshot/model parsing. |
| tests/fixtures/cct.json | Adds real-world CCT fixture for snapshot/model parsing. |
| tests/fixtures/rgb_websocket.json | Adds fixture variant for websocket-enabled device parsing. |
| tests/fixtures/rgb_single_segment.json | Adds single-segment fixture variant for parsing. |
| src/wled/wled.py | Fixes f-string bug and improves wording/exception handling/docstrings. |
| src/wled/models.py | Improves typing comments, docstrings, and strict zip behavior in playlist parsing. |
| src/wled/exceptions.py | Docstring capitalization/wording tweak. |
| src/wled/const.py | Docstring wording/capitalization tweaks. |
| src/wled/cli/async_typer.py | Typing import cleanup and updated type-ignore strategy for ty. |
| src/wled/cli/init.py | Adjusts scan display to handle missing/empty MAC bytes (but needs a guard). |
| pyproject.toml | Migrates project metadata/tool config, adds new dev tools and stricter coverage. |
| README.md | Updates badges and development instructions (prek, tooling changes). |
| examples/upgrade.py | Docstring grammar fix. |
| LICENSE.md | Updates copyright year. |
| sonar-project.properties | Removes SonarCloud configuration. |
| .yamllint | Disables line-length rule and removes .venv ignore. |
| .pre-commit-config.yaml | Updates hook stages, adds zizmor, and swaps mypy → ty. |
| .gitignore | Removes mypy cache ignores and adds .ty/. |
| .editorconfig | Updates indentation rules (currently contains invalid key names). |
| .devcontainer/devcontainer.json | Updates devcontainer commands and Python feature version. |
| .github/workflows/tests.yaml | Expands Python matrix, pins action SHAs, adjusts coverage upload. |
| .github/workflows/typing.yaml | Switches typing job to ty, pins actions, sets permissions. |
| .github/workflows/linting.yaml | Moves pre-commit invocations to prek and adds zizmor job. |
| .github/workflows/release.yaml | Adds SBOM/attestations and modernizes/pins release steps. |
| .github/workflows/release-drafter.yaml | Pins action and tightens permissions. |
| .github/workflows/pr-labels.yaml | Pins action, permissions hardening, adds zizmor annotation. |
| .github/workflows/labels.yaml | Pins actions and hardens permissions. |
| .github/workflows/lock.yaml | Pins action and sets explicit permissions. |
| .github/workflows/scorecard.yml | Adds OpenSSF Scorecard workflow with SARIF upload. |
| .github/workflows/dependency-review.yaml | Adds dependency review workflow. |
| .github/workflows/requirements.txt | Adds pinned workflow tool versions for CI usage. |
| .github/workflows/stale.yaml | Removes stale-issue automation workflow. |
| .github/workflows/codeql.yaml | Removes CodeQL workflow (replaced by scorecard/zizmor additions). |
| .github/renovate.json | Updates Renovate configuration to pin GitHub Action digests. |
| .github/release-drafter.yml | Collapses dependency update section after N entries. |
| .github/SECURITY.md | Adds a security policy document. |
| .github/PULL_REQUEST_TEMPLATE.md | Fixes “GitHub” capitalization in template. |
| .github/ISSUE_TEMPLATE.md | Removes legacy issue template. |
| .github/CONTRIBUTING.md | Minor grammar/punctuation fixes. |
| .github/CODE_OF_CONDUCT.md | Updates to Contributor Covenant 3.0 text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Use info.properties.get(b"mac") instead of direct key access to avoid KeyError if mDNS record lacks mac property - Fix comment in Playlist.__post_deserialize__: "preset ID" -> "playlist ID" - Fix .editorconfig: ident_size/ident_style -> indent_size/indent_style Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wled/models.py (1)
683-700:⚠️ Potential issue | 🟠 MajorHandle the official WLED playlist field name
transitioninstead oftransitions.WLED's official JSON API uses
transition(singular) for playlist entry transitions, nottransitions(plural). Real WLED devices sending this standard field will have their transition values silently ignored and default to0. The code should check for"transition"first, with"transitions"as a fallback for backwards compatibility with existing test data.Proposed fix
- # If the transition value doesn't exist, we will set it to 0. - if "transitions" not in d: - d["transitions"] = [0] * item_count - # If the transition is a single value, we will convert it to a list. - # with the same length as the presets list. - elif not isinstance(d["transitions"], list): - d["transitions"] = [d["transitions"]] * item_count + # WLED API uses `transition` for playlist entry transitions. Fallback + # to `transitions` for backwards compatibility with existing test data. + transitions = d.get("transition", d.get("transitions", 0)) + if not isinstance(transitions, list): + transitions = [transitions] * item_count # Now we can easily combine the data into a list of dictionaries. d["entries"] = [ { "entry_id": entry_id, "ps": ps, "dur": dur, "transition": transition, } for entry_id, (ps, dur, transition) in enumerate( - zip(d["ps"], d["dur"], d["transitions"], strict=True) + zip(d["ps"], d["dur"], transitions, strict=True) ) ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wled/models.py` around lines 683 - 700, The code currently only checks for d["transitions"] and uses it when building d["entries"], which ignores the official WLED field "transition"; update the logic in the block that prepares d["transitions"]/d["transition"] (the lines creating the list when "transitions" not in d and the subsequent zip used in building d["entries"]) to first look for d["transition"] (singular), normalize it to a list of length item_count (or use the scalar-to-list fallback), and only fall back to d["transitions"] (plural) for backwards compatibility so the enumerate/zip in entries uses the correct transition values.
🧹 Nitpick comments (4)
.yamllint (1)
51-51: Prefer a softline-lengthcap instead of fully disabling it.
line-length: disableremoves a useful readability check globally. Consider keeping it as a warning with exceptions for non-breakable values (URLs/hash strings) so CI stays practical without dropping the rule entirely.♻️ Suggested tweak
- line-length: disable + line-length: + level: warning + max: 120 + allow-non-breakable-words: true + allow-non-breakable-inline-mappings: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.yamllint at line 51, Replace the global "line-length: disable" with a soft rule by setting "line-length" to include a reasonable "max" (e.g. 120), "level: warning", and "allow-non-breakable-strings: true" so long non-breakable tokens (URLs/hashes) are excluded; update the .yamllint rule named "line-length" accordingly and optionally add targeted "ignore" patterns if needed..github/CODE_OF_CONDUCT.md (1)
29-29: Optional: tighten a few wordy phrases for readability.Small editorial pass could simplify phrasing on Line 29, Line 48, and Line 71 without changing meaning.
✍️ Suggested wording tweaks
-3. **Stereotyping or discrimination.** Characterizing anyone's personality or behavior on the basis of immutable identities or traits. +3. **Stereotyping or discrimination.** Characterizing anyone's personality or behavior based on immutable identities or traits. -... In order to honor these values, enforcement actions are carried out in private ... +... To honor these values, enforcement actions are carried out in private ... -... to use their discretion and judgment, in keeping with the best interests of our community. +... to use their discretion and judgment, consistent with the best interests of our community.Also applies to: 48-48, 71-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/CODE_OF_CONDUCT.md at line 29, The sentence "Stereotyping or discrimination. Characterizing anyone's personality or behavior on the basis of immutable identities or traits." is wordy—please simplify phrasing for clarity (e.g., "Stereotyping or discrimination: attributing personality or behavior to immutable characteristics."). Apply a similar light editorial tightening to the other two instances flagged (the phrases on the same file at the other locations), keeping meaning intact and preserving the guideline tone.pyproject.toml (1)
31-31:packagesis in the wrong table.
packagesis not a standardized[project]key, and the spec reserves tool-specific configuration for[tool]. If you still need explicitsrcmapping, move this totool.poetryinstead ofproject. (packaging.python.org)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 31, The pyproject.toml currently places the packages = [{ include = "wled", from = "src" }] entry under the [project] table but that key is not part of the standardized metadata; move this packages mapping into the tool-specific section (e.g., under [tool.poetry] or another tool.* table your build tool uses) so it is no longer in [project]; locate the packages declaration in pyproject.toml, remove it from [project], and add the equivalent mapping under the appropriate [tool.poetry] (or the tool.* config you use) section so the package source mapping (include/from) is declared as tool-specific configuration.src/wled/cli/async_typer.py (1)
54-76: Broaden these decorator types to accept sync callables too.The implementation handles both sync and async functions, but the annotation only advertises coroutine functions. That mismatch is already leaking into
tests/test_cli.pyas repeated# ty: ignore[invalid-argument-type]at sync call sites. Overloading these decorators for both shapes would keep the newtysetup useful without pushing suppressions downstream.Also applies to: 114-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wled/cli/async_typer.py` around lines 54 - 76, The decorator type signatures (e.g., callback) currently only accept coroutine callables; update them to also accept sync callables by broadening the generics or adding overloads: provide one overload that accepts Callable[_P, Coroutine[Any, Any, _R]] and another that accepts Callable[_P, _R] (or simply use Callable[_P, Any] for the wrapped callable) so the returned decorator preserves the same parameterization for both sync and async targets; apply the same change to the sibling decorator declaration around lines 114-134 so tests no longer need ty:ignore for sync call sites and type checkers accept both function shapes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.devcontainer/devcontainer.json:
- Line 53: The devcontainer update command runs "poetry install" but doesn't
include the CLI extra, causing imports in src/wled/cli/__init__.py (typer,
zeroconf) to be missing; update the updateContentCommand so the poetry step uses
the cli extra (i.e., change the poetry install invocation to include --extras
cli) so Codespaces/devcontainers install the CLI dependencies like CI does.
In @.github/workflows/release.yaml:
- Around line 60-72: The attest step is applying the wheel-only SBOM
(wled.cdx.json generated for ${ env.WHEEL_PATH }) to all artifacts under
"dist/*"; update the workflow so the actions/attest step either targets only the
wheel artifact (use subject-path: "${{ env.WHEEL_PATH }}" or the wheel filename)
or generate a separate SBOM for the source distribution and add a second attest
step that uses that SBOM against the sdist; modify the steps named "📝 Generate
CycloneDX SBOM" and "📝 Attest SBOM against built artifacts" accordingly so each
SBOM is produced for and attested against its corresponding artifact rather than
"dist/*".
In `@pyproject.toml`:
- Around line 30-39: Remove the conflicting dynamic declaration or the static
dependencies: delete the line 'dynamic = ["dependencies"]' if you intend to keep
the explicit '[project].dependencies' list, or instead remove the static
'dependencies = [...]' block and move those requirements into your
build-backend/tool config (e.g., setuptools_scm/poetry/backend) so they are
provided dynamically; also remove the non-PEP621 'packages = [{ include =
"wled", from = "src" }]' key from the [project] table and relocate package
discovery to the tool-specific configuration (e.g., [tool.poetry] or the
build-backend settings such as setuptools' configuration) or use the backend’s
package-discovery mechanism.
In `@src/wled/cli/__init__.py`:
- Around line 324-328: Replace the direct indexing of info.properties[b"mac"]
(which can raise KeyError) with a safe lookup using info.properties.get(b"mac")
and keep the existing None handling/decoding; update the mac assignment used in
table.add_row (the mac variable, info.properties lookup, and the mac.decode()
call) so it uses the .get result and falls back to "unknown" when None.
In `@tests/test_cli.py`:
- Around line 317-324: Add the pytest fixture decorator to stabilize terminal
output for the snapshot tests: annotate test_playlists_command (and the two
other snapshot tests in the same file that assert Rich-rendered tables/panels)
with `@pytest.mark.usefixtures`("stable_terminal") so they run with consistent
terminal width/color support; update the test function declarations (e.g.,
test_playlists_command, its _empty variant and the other snapshot test group
referenced) to include the decorator immediately above each def.
In `@tests/test_models.py`:
- Around line 461-523: Tests are using the wrong playlist key "transitions"
instead of the real API field "transition"; update the test payloads passed to
Playlist.from_dict (including the cases in test_playlist (ps/dur/transitions ->
ps/dur/transition), test_playlist_single_duration, and
test_playlist_single_transition) to use "transition" (singular) so the inputs
match the real WLED playlist shape and Playlist.from_dict will deserialize
correctly.
In `@tests/test_wled.py`:
- Around line 90-132: The tests mismatch WLED.request()'s retry behavior:
TimeoutError is converted into WLEDConnectionTimeoutError immediately (no
retries) while only WLEDConnectionError is retried. Update test_backoff to have
the first two mocked GETs raise WLEDConnectionError (so the third 200 response
is reached) and keep the third mocked GET as status 200; update test_timeout to
mock GETs that raise TimeoutError and assert that WLEDConnectionTimeoutError is
raised (instead of WLEDConnectionError). Target the test functions test_backoff
and test_timeout and the WLED.request() behavior when adjusting mocks and
assertions.
---
Outside diff comments:
In `@src/wled/models.py`:
- Around line 683-700: The code currently only checks for d["transitions"] and
uses it when building d["entries"], which ignores the official WLED field
"transition"; update the logic in the block that prepares
d["transitions"]/d["transition"] (the lines creating the list when "transitions"
not in d and the subsequent zip used in building d["entries"]) to first look for
d["transition"] (singular), normalize it to a list of length item_count (or use
the scalar-to-list fallback), and only fall back to d["transitions"] (plural)
for backwards compatibility so the enumerate/zip in entries uses the correct
transition values.
---
Nitpick comments:
In @.github/CODE_OF_CONDUCT.md:
- Line 29: The sentence "Stereotyping or discrimination. Characterizing anyone's
personality or behavior on the basis of immutable identities or traits." is
wordy—please simplify phrasing for clarity (e.g., "Stereotyping or
discrimination: attributing personality or behavior to immutable
characteristics."). Apply a similar light editorial tightening to the other two
instances flagged (the phrases on the same file at the other locations), keeping
meaning intact and preserving the guideline tone.
In @.yamllint:
- Line 51: Replace the global "line-length: disable" with a soft rule by setting
"line-length" to include a reasonable "max" (e.g. 120), "level: warning", and
"allow-non-breakable-strings: true" so long non-breakable tokens (URLs/hashes)
are excluded; update the .yamllint rule named "line-length" accordingly and
optionally add targeted "ignore" patterns if needed.
In `@pyproject.toml`:
- Line 31: The pyproject.toml currently places the packages = [{ include =
"wled", from = "src" }] entry under the [project] table but that key is not part
of the standardized metadata; move this packages mapping into the tool-specific
section (e.g., under [tool.poetry] or another tool.* table your build tool uses)
so it is no longer in [project]; locate the packages declaration in
pyproject.toml, remove it from [project], and add the equivalent mapping under
the appropriate [tool.poetry] (or the tool.* config you use) section so the
package source mapping (include/from) is declared as tool-specific
configuration.
In `@src/wled/cli/async_typer.py`:
- Around line 54-76: The decorator type signatures (e.g., callback) currently
only accept coroutine callables; update them to also accept sync callables by
broadening the generics or adding overloads: provide one overload that accepts
Callable[_P, Coroutine[Any, Any, _R]] and another that accepts Callable[_P, _R]
(or simply use Callable[_P, Any] for the wrapped callable) so the returned
decorator preserves the same parameterization for both sync and async targets;
apply the same change to the sibling decorator declaration around lines 114-134
so tests no longer need ty:ignore for sync call sites and type checkers accept
both function shapes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d87cf9a4-3eb2-405c-88f6-540b5ddd2b63
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
.devcontainer/devcontainer.json.editorconfig.github/CODE_OF_CONDUCT.md.github/CONTRIBUTING.md.github/ISSUE_TEMPLATE.md.github/PULL_REQUEST_TEMPLATE.md.github/SECURITY.md.github/release-drafter.yml.github/renovate.json.github/workflows/codeql.yaml.github/workflows/dependency-review.yaml.github/workflows/labels.yaml.github/workflows/linting.yaml.github/workflows/lock.yaml.github/workflows/pr-labels.yaml.github/workflows/release-drafter.yaml.github/workflows/release.yaml.github/workflows/requirements.txt.github/workflows/scorecard.yml.github/workflows/stale.yaml.github/workflows/tests.yaml.github/workflows/typing.yaml.gitignore.pre-commit-config.yaml.yamllintLICENSE.mdREADME.mdexamples/upgrade.pypyproject.tomlsonar-project.propertiessrc/wled/cli/__init__.pysrc/wled/cli/async_typer.pysrc/wled/const.pysrc/wled/exceptions.pysrc/wled/models.pysrc/wled/wled.pytests/__snapshots__/test_cli.ambrtests/__snapshots__/test_models.ambrtests/conftest.pytests/fixtures/cct.jsontests/fixtures/presets.jsontests/fixtures/rgb.jsontests/fixtures/rgb_single_segment.jsontests/fixtures/rgb_websocket.jsontests/fixtures/rgbw.jsontests/fixtures/wled.jsontests/test_cli.pytests/test_models.pytests/test_wled.py
💤 Files with no reviewable changes (4)
- .github/ISSUE_TEMPLATE.md
- sonar-project.properties
- .github/workflows/codeql.yaml
- .github/workflows/stale.yaml
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 50 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add missing @pytest.mark.usefixtures("stable_terminal") to
test_playlists_command, test_presets_command, and
test_scan_command_keyboard_interrupt for deterministic Rich output
- Add --extras cli to devcontainer poetry install so CLI dependencies
(typer, zeroconf) are available in Codespaces
- Narrow SBOM attestation subject-path from dist/* to the wheel only,
since the SBOM is generated from the wheel
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Proposed Changes
Related Issues
Summary by CodeRabbit
Infrastructure Updates
Security & Governance
Documentation