Skip to content

chore: Triage and clean up TODO comments - #473

Merged
currantw merged 33 commits into
valkey-io:mainfrom
currantw:325-todos
Jul 29, 2026
Merged

chore: Triage and clean up TODO comments#473
currantw merged 33 commits into
valkey-io:mainfrom
currantw:325-todos

Conversation

@currantw

@currantw currantw commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Summary

Triage and clean up TODO comments across the codebase. Adds a check-todos script and CI workflow to enforce that all TODOs reference an open GitHub issue with a description, then resolves or annotates all existing TODOs.

Issue Link

Closes #325.

Features and Behaviour Changes

TODO checker infrastructure

  • Add dev/scripts/check_todos.py — validates that every TODO comment follows the format TODO #<number>: <description>.
    • Description must be at least 10 characters.
    • Referenced issue must be open.
  • Add dev/conf/check-todos-ignore — fnmatch-based ignore file for paths excluded from validation.
  • Add .github/workflows/check-todos.yml — CI workflow that runs the checker on PRs and pushes.
  • Add task check:todos command.
  • Document the TODO format in DEVELOPER.md.

TODO comment fixes and removals

  • Request.StreamCommands.cs — Remove SER-specific StreamTrimAsync method; replace with GLIDE-style method accepting StreamTrimOptions. SER Database.StreamCommands now routes through the BaseClient.StreamTrimAsync GLIDE interface method.
  • IDatabaseAsync.StreamCommands.cs — Fix StreamTrimAsync signature to use non-nullable long maxLength (matching StackExchange.Redis 2.8.58).
  • IBaseClient.StreamCommands.cs / BaseClient.StreamCommands.cs — Uncomment and implement StreamTrimAsync(ValkeyKey, StreamTrimOptions).
  • ScanOptions.cs — Add seealso links for SCAN, SSCAN, and ZSCAN commands.
  • Request.cs — Extract shared ToValkeyKeyArray and ToValkeyValueArray collection converters.
  • Request.SetCommands.cs — Extract ConvertSetScanResponse for SSCAN response parsing.
  • Request.SortedSetCommands.cs — Extract ConvertSortedSetScanResponse and ToSortedSetEntriesFromFlatArray for ZSCAN response parsing; remove old ToValkeyValues delegate.
  • Request.GenericCommands.cs — Extract ConvertClusterScanResponse for top-level SCAN (string cursor for cluster compatibility).
  • GlideClusterClient.cs — Refactor ClusterScanCommand to accept ClusterScanCursor instead of raw string; simplify ScanAsync loop.
  • GlideString.cs — Replace round-trip UTF-8 encode/decode with Utf8.IsValid (net8.0 API, allocation-free).
  • Helpers.cs — Remove dead code (DownCastVals, ConvertKeyValuePairsToArray).
  • GlideClient.cs / GlideClusterClient.cs — Remove // TODO add pubsub comments; docs already updated in prior commits.
  • ConnectionMultiplexer.cs — Remove TODO referencing closed upstream issue ([Feature request] Routing on standalone valkey-glide#4293).
  • FFI.structs.cs — Replace vague TODO with issue reference (TODO #472).
  • Valkey.Glide.nuspec — Resolve all packaging TODOs: add <releaseNotes>, commit attribute, remove stale comments.
  • cd.yml — Pass commit=${{ github.sha }} to nuspec via NuspecProperties.
  • Integration/unit tests — Remove resolved TODO skip markers; update CommandTests for new StreamTrimAsync signature.

Cleanup

  • Move dev/coverage/coverage-baseline.json to dev/conf/coverage-baseline.json.
  • Move dev/coverage/.runsettings to dev/conf/coverage-runsettings.xml.
  • Remove empty dev/coverage/ directory (output subdirectories are recreated at runtime).
  • Update all references in CI workflows, scripts, and documentation.

Implementation

The main design decisions:

  1. Stream trim refactoring: SER methods now build StreamTrimOptions (MaxLen or MinId) and call through the GLIDE BaseClient.StreamTrimAsync method, eliminating direct Request calls from the SER layer.
  2. Scan response converters: Each scan command (SCAN, SSCAN, ZSCAN) has its own dedicated converter method rather than a shared generic — simpler to read and avoids unnecessary indirection.
  3. TODO checker: Uses git grep for discovery, gh issue view for validation, and fnmatch patterns for exclusions. Fails hard if the ignore file is missing.

Limitations

⚪ None

Testing

  • All 547 unit tests pass.
  • Integration tests not run locally (require a running Valkey instance).
  • TODO checker validates all 25 remaining TODOs pass format and issue checks.

Related Issues

Checklist

  • This Pull Request is related to one issue.
  • Commit message has a detailed description of what changed and why.
  • Tests are added or updated and all checks pass.
  • CHANGELOG.md, README.md, DEVELOPER.md, and other documentation files are updated.
  • Destination branch is correct - main or release
  • Create merge commit if merging release branch into main, squash otherwise.
  • All TODO comments referencing issue(s) closed by this PR have been resolved.

@currantw currantw self-assigned this Jul 23, 2026
@currantw currantw changed the title chore: Triage and clean up TODO comments (#325) chore: Triage and clean up TODO comments Jul 23, 2026
@currantw currantw added docs Documentation and developer guides ci CI/CD pipelines and GitHub Actions dev Internal changes like cleanup, refactoring, or developer tools. labels Jul 23, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces repository-wide enforcement for TODO comment formatting (including validating referenced GitHub issues), while also cleaning up and refactoring several command implementations (notably stream trim and scan variants) and updating related docs/CI configuration.

Changes:

  • Added a TODO checker (dev/scripts/check_todos.py), ignore configuration, Taskfile integration, and a dedicated CI workflow to enforce TODO #<issue>: <description> formatting.
  • Refactored stream trim and scan command plumbing (request builders, converters, and client/DB/IServer iteration patterns) and updated unit/integration tests accordingly.
  • Cleaned up TODOs and updated documentation/CI paths (coverage config relocation, nuspec metadata, workflow/task renames).

Reviewed changes

Copilot reviewed 45 out of 47 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/Valkey.Glide.UnitTests/CommandTests.cs Updates stream trim request arg/converter tests to use StreamTrimOptions.
tests/Valkey.Glide.IntegrationTests/StreamCommandTests.cs Removes commented legacy TODO markers in stream integration tests.
tests/Valkey.Glide.IntegrationTests/StandaloneClientTests.cs Removes TODO commentary from an integration test.
tests/Valkey.Glide.IntegrationTests/SortedSetCommandTests.cs Replaces version-gated skips with centralized Skip helpers.
tests/Valkey.Glide.IntegrationTests/Skip.cs Renames/organizes version-based skip helpers and messages.
tests/Valkey.Glide.IntegrationTests/SharedCommandTests.cs Removes a TODO and keeps current assertion behavior.
tests/Valkey.Glide.IntegrationTests/SharedBatchTests.cs Replaces custom-command placeholders with concrete batch command calls.
tests/Valkey.Glide.IntegrationTests/HashCommandTests.cs Removes a TODO comment from the test class header.
Taskfile.yml Adds task check umbrella and renames check tasks; adds --yes to npx invocations.
sources/Valkey.Glide/Valkey.Glide.nuspec Adds repo commit metadata + release notes; clarifies native binary packaging expectations.
sources/Valkey.Glide/Internals/Request.StreamCommands.cs Replaces legacy stream trim request builder with options-based trim args.
sources/Valkey.Glide/Internals/Request.SortedSetCommands.cs Refactors sorted set scan response conversion and reuses shared value-array converter.
sources/Valkey.Glide/Internals/Request.SetCommands.cs Refactors set scan response conversion and reuses shared value-array converter.
sources/Valkey.Glide/Internals/Request.GenericCommands.cs Extracts cluster scan response converter and reuses shared key-array converter.
sources/Valkey.Glide/Internals/Request.cs Removes internal SCAN arg builder; adds shared key/value array converters.
sources/Valkey.Glide/Internals/Helpers.cs Removes dead helpers and adds SkipAsync for async sequences.
sources/Valkey.Glide/Internals/FFI.structs.cs Replaces vague TODOs with an issue-linked TODO and removes a stale config TODO.
sources/Valkey.Glide/GlideString.cs Uses allocation-free UTF-8 validity checking before decoding.
sources/Valkey.Glide/Errors.cs Removes a stale TODO from ConfigurationError.
sources/Valkey.Glide/Commands/Options/ScanOptions.cs Adds docs links and moves scan argument construction onto ScanOptions.
sources/Valkey.Glide/Client/IBaseClient.StreamCommands.cs Adds and documents the StreamTrimAsync(key, StreamTrimOptions) API.
sources/Valkey.Glide/Client/GlideClusterClient.cs Simplifies scan loop and introduces ClusterScanCursor usage.
sources/Valkey.Glide/Client/GlideClient.cs Simplifies scan loop and removes outdated doc TODO content.
sources/Valkey.Glide/Client/BaseClient.StreamCommands.cs Implements StreamTrimAsync(key, StreamTrimOptions) through Request.
sources/Valkey.Glide/Client/BaseClient.SortedSetCommands.cs Refactors sorted set scan iteration variable naming and loop structure.
sources/Valkey.Glide/Client/BaseClient.SetCommands.cs Refactors set scan iteration variable naming and loop structure.
sources/Valkey.Glide/Abstract/ValkeyServer.cs Refactors KeysAsync to use ScanOptions + async sequence skipping.
sources/Valkey.Glide/Abstract/IDatabaseAsync.StreamCommands.cs Adjusts stream trim overload signature (non-nullable long maxLength) and formatting.
sources/Valkey.Glide/Abstract/Database.StreamCommands.cs Routes stream trim through StreamTrimOptions and updates overload wiring.
sources/Valkey.Glide/Abstract/Database.SortedSetCommands.cs Refactors SortedSetScanAsync to use ScanOptions + async sequence skipping.
sources/Valkey.Glide/Abstract/Database.SetCommands.cs Refactors SetScanAsync to use ScanOptions + async sequence skipping.
sources/Valkey.Glide/Abstract/ConnectionMultiplexer.cs Cleans up TODO commentary and ensures subscription disposal is enclosed in braces.
docs/coverage.md Updates coverage baseline paths and documents new conf layout.
DEVELOPER.md Updates developer commands, adds TODO format documentation, and reorganizes checks/lint/format sections.
dev/scripts/get_licenses_from_ort.py Removes a TODO about license expression parsing.
dev/scripts/check_todos.py Adds new TODO-format and issue-state validation script.
dev/scripts/check_examples.py Updates docstring to match new script path and clarifies behavior.
dev/scripts/_constants.py Adds repo constant and moves coverage conf paths under dev/conf.
dev/conf/coverage-runsettings.xml Adds relocated coverage runsettings file.
dev/conf/coverage-baseline.json Adds relocated coverage baseline file.
dev/conf/check-todos-ignore Adds ignore patterns for the TODO checker.
AGENTS.md Updates quality gate checklist to reference the new task check and docs anchors.
.github/workflows/test.yml Updates coverage baseline path references.
.github/workflows/check-todos.yml Adds CI workflow to run TODO validation.
.github/workflows/check-examples.yml Updates task invocation to task check:examples.
.github/workflows/cd.yml Passes git SHA into nuspec properties as commit.
.github/pull_request_template.md Adds a checkbox ensuring TODOs referencing closed issues are resolved.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread sources/Valkey.Glide/Abstract/ValkeyServer.cs Outdated
Comment thread sources/Valkey.Glide/Abstract/Database.SetCommands.cs Outdated
Comment thread sources/Valkey.Glide/Abstract/Database.SortedSetCommands.cs Outdated
Comment thread sources/Valkey.Glide/Abstract/Database.StreamCommands.cs Outdated
@currantw

Copy link
Copy Markdown
Collaborator Author

Sample output from recent run:

task: [check:todos] python3 dev/scripts/check_todos.py
Checking TODO comments...

  FAIL  sources/Valkey.Glide/Client/BaseClient.StreamCommands.cs:98
        missing description (expected format: TODO #<number>: <description>)

Checked 27 TODOs: 26 passed, 1 failed.
task: Failed to run task "check:todos": exit status 1
Error: exit status 1

Error: Process completed with exit code 201.

currantw added 14 commits July 23, 2026 14:03
Add a Python script that validates all TODO comments reference an open
GitHub issue. TODOs must use the format `TODO #<number>` inside a comment
marker (// # /* * <!--). The script checks each referenced issue via the
`gh` CLI and reports distinct errors for missing references, invalid IDs,
and closed issues.

- Add `dev/scripts/check_todos.py` with `_find_todos` and `_validate_todos`
- Add `task check-todos` to Taskfile.yml
- Add `.github/workflows/check-todos.yml` CI workflow
- Add TODO resolution checklist item to PR template
- Add `GITHUB_REPO` constant to `dev/scripts/_constants.py`
- Update AGENTS.md quality gates with task commands
- Consolidate DEVELOPER.md lint/format/checks section

Addresses GitHub issue valkey-io#325

Signed-off-by: Taylor Curran <taylor.curran@valkey.io>
Signed-off-by: currantw <taylor.curran@improving.com>
- Rename check-examples, check-links, check-todos to check:examples,
  check:links, check:todos (consistent with lint:* and format:*)
- Add `task check` aggregator to run all checks in parallel
- Split DEVELOPER.md section into separate Formatting, Linting, and
  Checks headings
- Update AGENTS.md quality gates with separate section references
- Update workflow files to use new task names

Signed-off-by: Taylor Curran <taylor.curran@valkey.io>
Signed-off-by: currantw <taylor.curran@improving.com>
npx prompts for confirmation when a package isn't cached, which stalls
CI and automated runs. Adding --yes auto-accepts the install.

Signed-off-by: Taylor Curran <taylor.curran@valkey.io>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Add dev/conf/check-todos-ignore to specify files excluded from TODO
validation using fnmatch glob patterns. This prevents the checker from
flagging its own comments.

Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Move coverage-baseline.json and .runsettings from dev/coverage/ to
dev/conf/ (renamed to coverage-runsettings.xml) to consolidate config
files. Update all references in CI workflows, scripts, and docs.

Also add TODO format documentation and description validation to the
check-todos script.

Signed-off-by: currantw <taylor.curran@improving.com>
- Add protected cursor-accepting overloads to BaseClient for
  SetScanAsync and SortedSetScanAsync
- Add internal ScanKeysAsync to GlideClient for key scanning
- SER scan methods now delegate to GLIDE methods with SkipAsync
  for pageOffset support
- ValkeyServer.KeysAsync uses private ScanWithRouteAsync to
  preserve per-node routing via MakeRoute()

Signed-off-by: Taylor Curran <taylor.curran@valkey.io>
Signed-off-by: currantw <taylor.curran@improving.com>
The int overload of StreamTrimAsync called itself due to parameter
resolution. Cast to long to target the correct overload, and add
ThrowIfNegative validation for maxLength.

- Add GuardClauses.ThrowIfNegative(long, string)
- Add GuardClauses.ThrowIfUnsupported(StreamTrimMode)
- Move ThrowIfUnsupportedTrimMode logic to GuardClauses

Signed-off-by: Taylor Curran <taylor.curran@valkey.io>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Remove the comment-prefix requirement from _TODO_PATTERN so TODOs
inside block comments (e.g. /* TODO */) are also caught. Handle
false positives from documentation files via check-todos-ignore.

Also removes the stale TODO from rust/src/ffi.rs (fields are now
implemented).

Signed-off-by: Taylor Curran <taylor.curran@valkey.io>
Signed-off-by: currantw <taylor.curran@improving.com>
Comment thread .github/workflows/cd.yml
Comment thread dev/conf/coverage-baseline.json
Comment thread dev/scripts/get_licenses_from_ort.py
Comment thread sources/Valkey.Glide/Abstract/ConnectionMultiplexer.cs
Comment thread dev/scripts/check_todos.py Outdated
Comment thread sources/Valkey.Glide/Errors.cs
Comment thread rust/src/ffi.rs
Comment thread sources/Valkey.Glide/Abstract/IDatabaseAsync.StreamCommands.cs
Comment thread sources/Valkey.Glide/Abstract/Database.SetCommands.cs
Comment thread dev/scripts/check_todos.py
Comment thread dev/scripts/check_todos.py Outdated
Comment thread dev/scripts/check_todos.py Outdated
Comment thread dev/scripts/check_todos.py
Comment thread sources/Valkey.Glide/Client/BaseClient.SetCommands.cs
currantw added 6 commits July 27, 2026 09:54
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
…if negative page offset

Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
currantw added 2 commits July 27, 2026 12:27
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
currantw added 2 commits July 27, 2026 13:10
Keep the previous `long? maxLength = null` signature on the extended
StreamTrimAsync overload to maintain backward compatibility. Add
ArgumentNullException for null maxLength (previously would have hit a
server syntax error). Add TODO valkey-io#486 to align with SER signature in 2.0.

Signed-off-by: Taylor Curran <taylor.curran@valkey.io>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
currantw requested a review from xShinnRyuu July 27, 2026 20:19
Comment thread dev/scripts/check_todos.py Outdated
Signed-off-by: currantw <taylor.curran@improving.com>

@jamesx-improving jamesx-improving left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup — check_todos + CI wiring is a good forcing function, and the SER→GLIDE stream-trim refactor reads well. One inline note on the SkipAsync change.

Happy to re-review once you've had a look.

Comment thread sources/Valkey.Glide/Internals/Helpers.cs
currantw added 4 commits July 27, 2026 21:09
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
…ches!)

Signed-off-by: currantw <taylor.curran@improving.com>

@jamesx-improving jamesx-improving left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs match the implementation now and the pageOffset > pageSize tests pin the choice — LGTM.

…edis

GLIDE skips pageOffset elements from the entire result stream
(deterministic), unlike SE.Redis which only skips within the first
SCAN batch (non-deterministic since batch sizes are server-controlled).

- Update XML docs on SetScanAsync, SortedSetScanAsync, and KeysAsync
- Add integration tests for pageOffset > pageSize edge case
- Remove duplicate KeysAsync_CommandFlags_Throws (already in CommandFlagsTests)

Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
currantw merged commit b2c4a6f into valkey-io:main Jul 29, 2026
14 checks passed
@currantw
currantw deleted the 325-todos branch July 29, 2026 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci CI/CD pipelines and GitHub Actions dev Internal changes like cleanup, refactoring, or developer tools. docs Documentation and developer guides

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore: Triage and clean up TODO comments across the codebase

4 participants