-
Notifications
You must be signed in to change notification settings - Fork 166
Conversation
Move bats test to dedicated dir
Just a couple notes:
|
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.
Regarding 1
: I think it's ok to not implement it's use case for requiredHash
yet, but would like a test. So, for example, it'll be obvious if https://github.com/ethereumproject/go-ethereum/pull/558/files#diff-5f06923a4803a1d0c02fce9f582d0495R36 is intentional or not.
Regarding 2
: according to https://github.com/fsnotify/fsnotify#faq the watcher does not watch directories recursively, so if pointed at <datadir>/<chaindir>/
we don't have to worry about filtering a ton of chaindata/
events or anything like that. 👍
Regarding 3
: r8d8 has notified me that maintainers have been given push permissions to the PR branch.
core/cfgwatcher.go
Outdated
log.Println("event:", event) | ||
if event.Op&fsnotify.Write == fsnotify.Write { | ||
log.Println("modified file:", event.Name) | ||
//cw.notify <- event |
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.
Is this supposed to be commented?
"github.com/fsnotify/fsnotify" | ||
) | ||
|
||
type configWatchdog struct { |
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.
I might suggest to rename this (and the file) to something more general instead of "config*", like maybe dirWatchdog
/dirwatcher.go
? -- the functionality implemented could be used to watch any directory.
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.
yeap, sounds reasonable
@@ -343,15 +345,17 @@ func main() { | |||
// It creates a default node based on the command line arguments and runs it in | |||
// blocking mode, waiting for it to be shut down. | |||
func geth(ctx *cli.Context) error { | |||
//_, err := core.NewConfigWatchdog("") |
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.
A leftover debugging comment?
core/cfgwatcher.go
Outdated
log.Println("modified file:", event.Name) | ||
//cw.notify <- event | ||
} | ||
case err := <-cw.watcher.Errors: |
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.
We'll want to handle watcher errors besides logging, too. Maybe they just get sent thru the cw.notify
chan too, then we'll check for and handle them during event notifier filtering?
case event := <-cw.watcher.Events: | ||
log.Println("event:", event) | ||
if event.Op&fsnotify.Write == fsnotify.Write { | ||
log.Println("modified file:", event.Name) |
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.
Let's stick with using glog.[D|V]
plz ;)
|
||
// Stop config directory watching | ||
func (cw *configWatchdog) Stop() error { | ||
return cw.watcher.Close() |
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.
Should we also close cw.notify
chan? Go channels typically won't cause problems if they're not explicitly closed, but would encourage and force us to be explicit about channel flow.
|
||
// Start config directory watching | ||
func (cw *configWatchdog) Start() { | ||
go func() { |
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.
According to https://github.com/fsnotify/fsnotify#faq -> howeyc/fsnotify#7 -> golang/go#8282 (comment) it might be unsafe to watch Events and Errors in the same go routine.
* config file watching, "requiedHash" support * vendor fsnotify (and update some dependencies)
No description provided.