Skip to content

Conversation

@chrisguidry
Copy link
Owner

Nate's PR fixing xpending_range has been merged into the main fakeredis repo, so we can now switch from his fork to the official repository.

Changes

  • Moved fakeredis from dev dependencies to main dependencies (it's used for the memory backend feature, not just testing)
  • Updated from Nate's fork to main repo at commit ad50a0d
  • This commit includes the xpending_range fix that returns all 4 required fields (message_id, consumer, time_since_delivered, times_delivered)
  • Added comment noting we're using a specific commit until version > 2.32.0 is released (presumably 2.33.0)

Testing

Verified with full test suite using memory backend - all 272 tests pass, including the 4 memory backend specific tests.

Related: cunla/fakeredis-py#427

🤖 Generated with Claude Code

Nate's PR fixing xpending_range has been merged into the main fakeredis
repo, so we can now switch from his fork to the official repository.

Changes:
- Moved fakeredis from dev dependencies to main dependencies (it's used
  for the memory backend feature, not just testing)
- Updated from Nate's fork to main repo at commit ad50a0d
- This commit includes the xpending_range fix that returns all 4 required
  fields (message_id, consumer, time_since_delivered, times_delivered)
- Added comment noting we're using a specific commit until version > 2.32.0
  is released (presumably 2.33.0)

Related: cunla/fakeredis-py#427

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@claude
Copy link
Contributor

claude bot commented Oct 31, 2025

Claude finished @chrisguidry's task —— View job


Pull Request Review

I've completed my review of this PR that updates the fakeredis dependency from a fork to the main repository.

Tasks:

  • Analyze the dependency changes in pyproject.toml
  • Review the lock file updates
  • Understand fakeredis usage in the codebase
  • Verify the migration from fork to main repo is appropriate
  • Check if moving fakeredis from dev to main dependencies is justified
  • Assess security and performance implications
  • Verify test coverage requirements

Review Summary

Strengths:

  1. Proper dependency categorization: Moving fakeredis from dev dependencies to main dependencies is correct. The codebase uses fakeredis at runtime for the memory:// URL feature (src/docket/docket.py:190-208), not just for testing. This is a production feature that allows users to run Docket with an in-memory backend.

  2. Good documentation: The commit includes clear comments explaining why a specific commit is being used instead of a released version, with a link to the relevant PR (#427) that fixed the xpending_range issue.

  3. Graceful handling: The code properly handles the optional nature of fakeredis with a try/except ImportError block, providing a helpful error message if users try to use memory:// URLs without the dependency installed.

  4. Upstream adoption: Moving from a fork to the main repository is the right decision now that the fix has been merged upstream. This improves maintainability and security.

📝 Observations:

  1. Pinning to commit hash: While pinning to a specific commit (ad50a0de8d6dce554fb629ec284bc4ccbc6a7f12) ensures you get the exact fix needed, consider setting up a reminder to update to the official release (>2.32.0) when it becomes available. This will make dependency management cleaner.

  2. Test coverage: The PR description mentions all 272 tests pass. The memory backend has dedicated tests in tests/test_memory_backend.py that properly skip if fakeredis is not installed using pytest.importorskip("fakeredis").

  3. No breaking changes: The migration from fork to main repo at the same functionality level should not introduce any breaking changes for users.

🔒 Security Considerations:

  • Moving to the official repository is more secure than using a fork, as it benefits from the main project's security reviews and updates
  • The specific commit being used includes a targeted fix without other potentially risky changes
  • No sensitive information or credentials are exposed in the changes

Performance Considerations:

  • No performance impact expected - this is essentially the same code, just from the official repository
  • The memory backend feature itself is appropriately isolated and only loaded when needed

Recommendation

This PR looks good to merge. The changes are well-justified, properly documented, and improve the project's dependency management by moving to the official upstream repository. The migration from dev to main dependencies correctly reflects the actual usage of fakeredis in production code.

Consider creating a GitHub issue to track updating to the next official fakeredis release (>2.32.0) when it becomes available, which will allow you to use a standard version specification instead of a git commit reference.

@chrisguidry chrisguidry requested a review from zzstoatzz October 31, 2025 18:44
@github-actions
Copy link

📚 Documentation has been built for this PR!

You can download the documentation directly here:
https://github.com/chrisguidry/docket/actions/runs/18982132593/artifacts/4433090322

@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (a5afb8b) to head (6a4364d).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #179   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           34        34           
  Lines         5216      5216           
  Branches       265       265           
=========================================
  Hits          5216      5216           
Flag Coverage Δ
python-3.10 100.00% <ø> (ø)
python-3.11 98.79% <ø> (ø)
python-3.12 100.00% <ø> (ø)
python-3.13 100.00% <ø> (ø)
python-3.14 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

:shipit:

@chrisguidry chrisguidry merged commit 147daeb into main Oct 31, 2025
25 checks passed
@chrisguidry chrisguidry deleted the update-fakeredis-dependency branch October 31, 2025 18:48
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.

4 participants