Skip to content
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

Separate traverse directory into another goroutine #9

Closed
wants to merge 1 commit into from

Conversation

wingyplus
Copy link

This commit move filepath.Walk into another goroutine to separate
layer between detect file changes and command execution. And separate
detect skip file/directory if file/folder has begin with dot.

@nattawitc
Copy link
Collaborator

I have 3 concerns about this.

  • the absence of time.Sleep(800 * time.Millisecond) might make the goroutine that walkthrough file run too frequently and consume too much CPU.
  • the default case in line 116 will keep the loop running without waiting for the actual change. this might consume CPU too.
  • the stop case in main file exit program immediately without waiting for the goroutine to properly exit.

@wingyplus
Copy link
Author

@nattawitc First and second points that you mentioned are fixed. For third point, I think it should add WaitGroup to wait it until exit.

This commit move filepath.Walk into another goroutine to separate
layer between detect file changes and command execution. And separate
detect skip file/directory if file/folder has begin with dot.
@wingyplus
Copy link
Author

All fixed

@nattawitc
Copy link
Collaborator

@AnuchitO thought?

@AnuchitO
Copy link
Owner

AnuchitO commented Mar 6, 2019

I agree it's a good idea. I also think about how to separate layer
the Watcher is responsible for a detect change and notifies whenever the file has changed.
the Commander is responsible for control flow
the Executor is responsible for executing and managing for the process
it just an idea right now.

For your pull request
"filepath.Walk into another goroutine to separate"

we might need it in the future when we have more trigger mechanism apart from File change (e.g. Interruption from a keyboard to force to rerun again). At the same time, by adding goroutine, while having the benefit of separating trigger and having the possibility to add multiple triggers, it also adds more complexity to the codebase and makes it harder to test

Right now, unit test of re very weak. I prefer to add more unittest first. before we add more complexity even though it looks trivial.

"And separate detect skip file/directory if file/folder has begin with a dot."

This is a good idea and very make it testable. I will apply this logic.
So, In future we will ignore watching the files that match patterns defined in .gitignore.

I very appreciate your contribution.
I will apply coding style from your pull request as well e.g. StopWalk, SkipFile error.
That add more readability into codebase.

@wingyplus
Copy link
Author

I think we already have a conclusion. So close this PR.

@wingyplus wingyplus closed this Mar 12, 2019
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