Skip to content

Conversation

@nxtcoder17
Copy link
Owner

@nxtcoder17 nxtcoder17 commented Dec 19, 2024

closes #26

@sourcery-ai
Copy link

sourcery-ai bot commented Dec 19, 2024

Reviewer's Guide by Sourcery

This PR fixes the watch behavior in the task runner by restructuring how commands are executed and watched. The main changes involve separating command execution logic, implementing proper task cancellation, and making the watch configuration optional.

Sequence diagram for task execution and watching

sequenceDiagram
    participant User
    participant TaskRunner
    participant Watcher
    participant Executor

    User->>TaskRunner: Start Task
    TaskRunner->>Executor: Execute Commands
    Executor-->>TaskRunner: Command Execution Result
    alt Watch Enabled
        TaskRunner->>Watcher: Initialize Watcher
        Watcher->>TaskRunner: File Change Detected
        TaskRunner->>Executor: Cancel and Restart Commands
    end
    Executor-->>TaskRunner: Final Execution Result
    TaskRunner-->>User: Task Completion Status
Loading

Updated class diagram for task and watch types

classDiagram
    class Task {
        Env EnvVar
        Watch *TaskWatch
        Requires []*Requires
    }
    class TaskWatch {
        *bool Enable
        Dirs []string
        OnlySuffixes []string
        IgnoreSuffixes []string
        ExcludeDirs []string
    }
    class ParsedTask {
        Shell []string
        WorkingDir string
        *TaskWatch Watch
        Env map[string]string
        Interactive bool
        Commands []ParsedCommandJson
    }
    Task --> TaskWatch
    ParsedTask --> TaskWatch
Loading

File-Level Changes

Change Details Files
Refactored task execution to support watching all commands
  • Split command execution logic into a separate runCommand function
  • Added context cancellation support for proper task cleanup
  • Implemented wait group to ensure all commands complete
  • Added goroutine for concurrent command execution
runner/run-task.go
Modified watch configuration structure
  • Made Watch field optional in Task and ParsedTask structs
  • Changed Enable field to pointer type for optional behavior
  • Added proper watch field propagation during task parsing
types/types.go
types/parsed-types.go
parser/parse-task.go
Updated dependencies and logging
  • Updated fwatcher dependency to newer version
  • Improved logging configuration for command execution
  • Cleaned up unused dependencies
go.mod
go.sum

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a 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 remove the large block of commented out code - it's no longer needed and makes the code harder to read
Here's what I looked at during the review
  • 🟡 General issues: 2 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

<-ctx.Done()
logger.Debug("fwatcher is closing ...")
watch.Close()
<-time.After(200 * time.Millisecond)
Copy link

Choose a reason for hiding this comment

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

suggestion: Magic numbers should be named constants

Define constants for the sleep durations (200ms, 100ms) with meaningful names to explain their purpose.

Suggested implementation:

const (
	// ShutdownGracePeriod is the time to wait after closing the file watcher
	// before exiting to allow pending operations to complete
	ShutdownGracePeriod = 200 * time.Millisecond
)

			<-ctx.Done()
			logger.Debug("fwatcher is closing ...")
			watch.Close()
			<-time.After(ShutdownGracePeriod)

Note: The constant should be placed at the package level (top of the file), outside of any function. If there are existing constant blocks in the file, you may want to add this constant there instead of creating a new block.

watch.Close()
<-time.After(200 * time.Millisecond)
logger.Info("CLOSING..................")
os.Exit(0)
Copy link

Choose a reason for hiding this comment

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

issue (bug_risk): Avoid calling os.Exit from goroutines

Signal termination through the context and let the main goroutine handle program shutdown for proper cleanup.

@nxtcoder17 nxtcoder17 merged commit eeedbf5 into master Dec 19, 2024
@nxtcoder17 nxtcoder17 deleted the fix/watching branch December 19, 2024 06:59
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.

[BUG] watch tasks is wrong, actually, as it only watches the last command

2 participants