Skip to content

Allow task registration before entering context manager#208

Merged
chrisguidry merged 1 commit intomainfrom
register-before-aenter
Nov 25, 2025
Merged

Allow task registration before entering context manager#208
chrisguidry merged 1 commit intomainfrom
register-before-aenter

Conversation

@chrisguidry
Copy link
Copy Markdown
Owner

Moves the task registry initialization from __aenter__ to __init__,
so you can now register tasks on a Docket before calling async with.
This helps with integration scenarios where tasks need to be configured
before establishing the Redis connection.

The change also means tasks persist after __aexit__, enabling re-entry
with the same task registry. Standard tasks (trace, fail, sleep) are
available immediately after constructing a Docket.

🤖 Generated with Claude Code

Moves the task registry initialization from `__aenter__` to `__init__`,
so you can now register tasks on a Docket before calling `async with`.
This helps with integration scenarios where tasks need to be configured
before establishing the Redis connection.

The change also means tasks persist after `__aexit__`, enabling re-entry
with the same task registry. Standard tasks (trace, fail, sleep) are
available immediately after constructing a Docket.

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

Co-Authored-By: Claude <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Nov 25, 2025

Claude finished @chrisguidry's task —— View job


Code Review for PR #208

Todo List

  • Read and analyze changes in src/docket/docket.py
  • Review new tests in tests/test_docket.py
  • Assess code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Review security concerns
  • Verify test coverage completeness
  • Provide comprehensive feedback

Overall Assessment

This PR makes a focused and well-executed change to move task registry initialization from __aenter__ to __init__. The implementation is clean, backwards-compatible, and well-tested.

✅ Code Quality and Best Practices

Strengths:

  • Clean implementation with minimal code changes (4 additions, 4 deletions)
  • Maintains existing API contract - no breaking changes
  • Follows the project's established patterns for task management
  • Proper use of type hints with dict[str, TaskFunction]

Code organization:

  • The change logically moves initialization to where data structures should be initialized (constructor)
  • Removes unnecessary cleanup (del self.tasks) from __aexit__ since the registry now persists

✅ Potential Bugs or Issues

No critical issues found. The implementation correctly:

  • Initializes the task registry with standard tasks in __init__ (src/docket/docket.py:202)
  • Maintains the same registry structure and access patterns
  • Preserves all existing functionality

Edge case handled well:

  • The registry now persists after __aexit__, which is actually beneficial for re-entry scenarios

✅ Performance Considerations

Positive impacts:

  • Slightly better performance by avoiding re-initialization on each context entry
  • No memory leaks - the registry is still properly bound to the Docket instance lifetime
  • No additional overhead introduced

Trade-offs:

  • Tasks remain in memory for the entire Docket lifetime (not just within context)
  • This is negligible since task functions are lightweight references

✅ Security Concerns

No security issues identified:

  • No changes to authentication, authorization, or data handling
  • Task validation still occurs during registration via validate_dependencies (src/docket/docket.py:293)
  • No exposure of sensitive information

✅ Test Coverage

Excellent test coverage with 74 new lines added:

  1. Standard tasks availability (test_standard_tasks_available_after_init):

    • Verifies trace, fail, sleep tasks are available immediately after __init__
  2. Pre-registration (test_register_task_before_aenter):

    • Confirms tasks can be registered before entering context
  3. Task usability (test_registered_task_usable_after_aenter):

    • Ensures pre-registered tasks work correctly inside context
  4. Persistence (test_tasks_persist_after_aexit):

    • Validates tasks remain after exiting context
  5. Re-entry (test_docket_reentry_preserves_tasks):

    • Tests that multiple entries preserve the task registry

All tests follow the project's testing patterns and maintain the required 100% coverage.

Suggestions for Improvement

  1. Documentation: Consider updating the class docstring to explicitly mention that tasks can be registered before entering the context manager:

    class Docket:
        """A Docket represents a collection of tasks that may be scheduled for later
        execution. With a Docket, you can add, replace, and cancel tasks.
        
        Tasks can be registered before entering the async context manager, allowing
        for flexible integration scenarios.
        
        Example:
        ...
  2. Consider adding a property for read-only access (optional):

    @property
    def registered_tasks(self) -> Mapping[str, TaskFunction]:
        """Read-only view of registered tasks."""
        return MappingProxyType(self.tasks)

    This would prevent accidental modification of the tasks dict from outside the class.

Conclusion

This is a well-designed change that improves the flexibility of the Docket API without introducing breaking changes. The implementation is clean, the tests are comprehensive, and the change aligns well with Python's initialization patterns. The PR successfully addresses the integration scenario mentioned in the description while maintaining backwards compatibility.

Recommendation: ✅ Approve - Ready to merge as-is. The suggestions above are optional enhancements that could be addressed in a future PR if desired.

@github-actions
Copy link
Copy Markdown

📚 Documentation has been built for this PR!

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

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Nov 25, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (bbfa452) to head (20e9cc0).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #208   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           48        48           
  Lines         7513      7545   +32     
  Branches       395       395           
=========================================
+ Hits          7513      7545   +32     
Flag Coverage Δ
python-3.10 100.00% <100.00%> (ø)
python-3.11 99.01% <100.00%> (+<0.01%) ⬆️
python-3.12 100.00% <100.00%> (ø)
python-3.13 100.00% <100.00%> (ø)
python-3.14 100.00% <100.00%> (ø)

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

Files with missing lines Coverage Δ
src/docket/docket.py 100.00% <100.00%> (ø)
tests/test_docket.py 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@chrisguidry chrisguidry merged commit a7c542b into main Nov 25, 2025
25 checks passed
@chrisguidry chrisguidry deleted the register-before-aenter branch November 25, 2025 18:25
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