Extract ExecutionContext to executor/context.rs module #14

2025-07-05T14:33:32-07:00

jwilger commented

2025-07-05 14:33:32 -07:00

(Migrated from github.com)

Summary

Extract ExecutionContext type to dedicated executor/context.rs module
Second small step in systematic executor.rs refactoring (2,956 lines → smaller modules)
Maintains all existing APIs and functionality - no breaking changes

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Refactoring (non-breaking change that improves code organization)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Performance improvement
Test improvement

Testing Checklist

Unit tests pass locally
Integration tests pass locally
Property-based tests pass locally
All examples compile and run successfully
Benchmarks show no performance regressions
Memory usage remains stable under load

Performance Impact

No performance impact expected - pure code organization change with identical runtime behavior.

Security Checklist

Input validation requirements reviewed
Authentication/authorization impacts assessed
Data encryption requirements verified
Sensitive data exposure risks evaluated
Audit logging requirements confirmed

Code Quality

Code follows established patterns and conventions
Complex logic is well-documented
Error handling is comprehensive
Resource cleanup is handled properly
Thread safety considerations addressed

Reviewer Checklist

Business logic changes reviewed for correctness
Performance implications assessed
Security implications reviewed
Test coverage adequate for changes
Documentation updated as needed
Breaking changes properly communicated
Migration guide provided if needed

Review Focus

This is a pure refactoring with no functional changes. Focus on:

Verify module organization makes sense
Confirm no behavioral changes
Check that imports/exports are correct
Validate this follows the systematic refactoring plan from PLANNING.md
This builds on PR #13 (retry module extraction)

🤖 Generated with Claude Code

## Summary - Extract ExecutionContext type to dedicated executor/context.rs module - Second small step in systematic executor.rs refactoring (2,956 lines → smaller modules) - Maintains all existing APIs and functionality - no breaking changes ## Type of Change - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [x] Refactoring (non-breaking change that improves code organization) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] Documentation update - [ ] Performance improvement - [ ] Test improvement ## Testing Checklist - [ ] Unit tests pass locally - [ ] Integration tests pass locally - [ ] Property-based tests pass locally - [ ] All examples compile and run successfully - [ ] Benchmarks show no performance regressions - [ ] Memory usage remains stable under load ## Performance Impact No performance impact expected - pure code organization change with identical runtime behavior. ## Security Checklist - [ ] Input validation requirements reviewed - [ ] Authentication/authorization impacts assessed - [ ] Data encryption requirements verified - [ ] Sensitive data exposure risks evaluated - [ ] Audit logging requirements confirmed ## Code Quality - [ ] Code follows established patterns and conventions - [ ] Complex logic is well-documented - [ ] Error handling is comprehensive - [ ] Resource cleanup is handled properly - [ ] Thread safety considerations addressed ## Reviewer Checklist - [ ] Business logic changes reviewed for correctness - [ ] Performance implications assessed - [ ] Security implications reviewed - [ ] Test coverage adequate for changes - [ ] Documentation updated as needed - [ ] Breaking changes properly communicated - [ ] Migration guide provided if needed ## Review Focus This is a pure refactoring with no functional changes. Focus on: - Verify module organization makes sense - Confirm no behavioral changes - Check that imports/exports are correct - Validate this follows the systematic refactoring plan from PLANNING.md - This builds on PR #13 (retry module extraction) 🤖 Generated with [Claude Code](https://claude.ai/code)

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed

2025-07-05 14:35:02 -07:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull Request Overview

This PR refactors the executor module by extracting the execution context and retry logic into their own modules and updating executor.rs to use these new abstractions. All public APIs and behavior remain unchanged.

Extracted ExecutionContext into executor/context.rs
Extracted RetryConfig and RetryPolicy into executor/retry.rs
Refactored executor.rs to use StreamDiscoveryContext, pull in the new modules, and streamline the command execution pipeline
Updated PLANNING.md to reflect completed refactoring steps and enforce the next-task reminder

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
eventcore/src/executor/retry.rs	New `retry` module with `RetryConfig` and `RetryPolicy`
eventcore/src/executor/context.rs	New `context` module with `ExecutionContext`
eventcore/src/executor.rs	Removed inlined retry/context, added module imports, and reorganized execution flow
PLANNING.md	Updated refactoring checklist and final reminder item

Comments suppressed due to low confidence (3)

eventcore/src/executor.rs:916

[nitpick] This helper method is missing a Rustdoc comment. Adding documentation here will improve discoverability and maintain consistency with the rest of the module.

    async fn read_streams_with_circuit_breaker_only(

PLANNING.md:327

[nitpick] This new checklist item uses extra quotes and bold formatting, which deviates from the existing list style. Consider matching the surrounding numbering and formatting conventions for clarity.

5. **"Review @PLANNING.md to identify next refactoring task and create new todo list to continue systematic refactoring"** (FINAL ITEM for refactoring work)

eventcore/src/executor/context.rs:22

The code references uuid::Uuid, uuid::Timestamp, and uuid::NoContext without importing them; add use uuid::{Uuid, Timestamp, NoContext}; at the top of this module to ensure it compiles.

            correlation_id: uuid::Uuid::new_v7(uuid::Timestamp::now(uuid::NoContext)).to_string(),

## Pull Request Overview This PR refactors the `executor` module by extracting the execution context and retry logic into their own modules and updating `executor.rs` to use these new abstractions. All public APIs and behavior remain unchanged. - Extracted `ExecutionContext` into `executor/context.rs` - Extracted `RetryConfig` and `RetryPolicy` into `executor/retry.rs` - Refactored `executor.rs` to use `StreamDiscoveryContext`, pull in the new modules, and streamline the command execution pipeline - Updated `PLANNING.md` to reflect completed refactoring steps and enforce the next-task reminder ### Reviewed Changes Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment. | File | Description | |--------------------------------------|--------------------------------------------------------------------------------------| | eventcore/src/executor/retry.rs | New `retry` module with `RetryConfig` and `RetryPolicy` | | eventcore/src/executor/context.rs | New `context` module with `ExecutionContext` | | eventcore/src/executor.rs | Removed inlined retry/context, added module imports, and reorganized execution flow | | PLANNING.md | Updated refactoring checklist and final reminder item | <details> <summary>Comments suppressed due to low confidence (3)</summary> **eventcore/src/executor.rs:916** * [nitpick] This helper method is missing a Rustdoc comment. Adding documentation here will improve discoverability and maintain consistency with the rest of the module. ``` async fn read_streams_with_circuit_breaker_only( ``` **PLANNING.md:327** * [nitpick] This new checklist item uses extra quotes and bold formatting, which deviates from the existing list style. Consider matching the surrounding numbering and formatting conventions for clarity. ``` 5. **"Review @PLANNING.md to identify next refactoring task and create new todo list to continue systematic refactoring"** (FINAL ITEM for refactoring work) ``` **eventcore/src/executor/context.rs:22** * The code references `uuid::Uuid`, `uuid::Timestamp`, and `uuid::NoContext` without importing them; add `use uuid::{Uuid, Timestamp, NoContext};` at the top of this module to ensure it compiles. ``` correlation_id: uuid::Uuid::new_v7(uuid::Timestamp::now(uuid::NoContext)).to_string(), ``` </details>

eventcore/src/executor.rs

					
				@ -813,0 +894,4 @@

				                CommandError::from(err)

				            })

				        } else {

				            warn!(

[nitpick] The logic for reading streams with and without timeouts is now split across two similar functions (read_streams_with_timeout_and_circuit_breaker and read_streams_with_circuit_breaker_only), leading to duplication. Consider unifying these into a single helper that takes an optional timeout parameter.

    /// Reads streams with optional timeout and circuit breaker protection.
    async fn read_streams_with_optional_timeout_and_circuit_breaker(

[nitpick] The logic for reading streams with and without timeouts is now split across two similar functions (`read_streams_with_timeout_and_circuit_breaker` and `read_streams_with_circuit_breaker_only`), leading to duplication. Consider unifying these into a single helper that takes an optional timeout parameter. ```suggestion /// Reads streams with optional timeout and circuit breaker protection. async fn read_streams_with_optional_timeout_and_circuit_breaker( ```

jwilger commented

2025-07-06 14:11:27 -07:00

(Migrated from github.com)

Closing this PR. Similar to PR #13, while ExecutionContext is still in executor.rs, the major functional refactoring in PR #10 has significantly changed the structure of the executor module. The benefit of extracting this small type is now minimal compared to the effort required to rebase and maintain this change.