-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor/watch and execute #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- adds tests to cmd executor to prevent it from happening in future
Reviewer's Guide by SourceryThis pull request refactors the command execution logic to fix major issues and adds tests to support it. The changes include a new way to handle multiple commands, a fix for interactive mode, and a new way to kill processes. Sequence diagram for the new command execution flowsequenceDiagram
participant M as Main
participant W as Watcher
participant E as Executor
participant P as Process
M->>W: Create Watcher
M->>E: Create Executor(s)
M->>W: WatchAndExecute(executors)
activate W
loop For each file change event
W->>W: Check ignore rules
W->>E: OnWatchEvent
E->>P: Start new command
P-->>E: Command completed
E->>P: Kill process
end
Note over W,E: Multiple executors can now run in parallel
M->>W: Context cancelled
W->>E: Stop
E->>P: Kill all processes
deactivate W
Class diagram for CmdExecutor refactoringclassDiagram
class CmdExecutor {
-logger *slog.Logger
-parentCtx context.Context
-commands []func(context.Context) *exec.Cmd
-interactive bool
-mu sync.Mutex
-kill func() error
+OnWatchEvent(ev Event) error
+Start() error
+Stop() error
}
class CmdExecutorArgs {
+Logger *slog.Logger
+Commands []func(context.Context) *exec.Cmd
+Interactive bool
}
note for CmdExecutor "Refactored to support multiple commands
and improved process management"
CmdExecutor ..> CmdExecutorArgs : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @nxtcoder17 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please expand the PR description to detail the specific issues that were fixed and the key improvements made in this refactoring
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- revamped executor model in [fwatcher](nxtcoder17/fwatcher#8)
fixes major issues with commands executions, and adds tests to support it
Summary by Sourcery
Bug Fixes: