Support multiple -directory flags#32
Conversation
A single -directory flag assumes a specific project structure wherein the app entry point and all of its dependencies are accessible in a single directory tree. However, this is not always the case. Consider mono-repos with multiple applications. Often, there are directories for each application and shared dependencies are in a different directory tree along side those applications. Additionally, applications may be categorized (for example backend, frontend, infrastructure, etc), which exacerbates the need for more flexibility in defining watchable directories. Allowing multiple -directory flags maintains the existing functionality and interface while also supporting different project organizations without having to create complicated and harder to maintain -exclude-dir configurations.
githubnemo
left a comment
githubnemo
left a comment
There was a problem hiding this comment.
Hey, thanks for the PR :)
If I understand you correctly then you want to add support for directory whitelists in addition to the existing blacklist approach (-exclude-dir). In that case the PR looks fine with the exception of some minor comments.
| } | ||
|
|
||
| // dirList must be unique, use a zero-byte struct map as a set. | ||
| (*d)[clean_value] = struct{}{} |
There was a problem hiding this comment.
I don't see why we would add a directory entry if value == "". Is this intended?
There was a problem hiding this comment.
So we can report the error later, which I believe maintains the existing functionality.
There was a problem hiding this comment.
But what's there to report? You'd simply have (*d)[""] = struct{}{}, which would also mean that the length of the dir list would be non-zero (although there was an invalid directory supplied) and .First() will return "".
There was a problem hiding this comment.
I've moved directory validation into validateFlags to be more clear.
daemon.go
Outdated
| flag_build_dir = &default_build_dir | ||
| } else { | ||
| fmt.Fprintf(os.Stderr, "-build-dir is required when specifying multiple watch directeries.\n") | ||
| os.Exit(1) |
There was a problem hiding this comment.
You might want to use log.Fatal() here which does both of these things in one call.
There was a problem hiding this comment.
No problem. I cut+pasted this from somewhere else so I was just following what existied.
| log.Fatal("watcher.Add():", err) | ||
| } | ||
| } | ||
| watchDirectories(watcher) |
There was a problem hiding this comment.
I think this is not the best way to refactor this. How about this:
func watchDirectory(dir string, watcher *fsnotify.Watcher) {
// ...
}
func main() {
// ...
for dir := range flag_directories {
watchDirectory(dir, watcher)
}
}There was a problem hiding this comment.
I structured it this way (along with validateFlags) with an eye on cleaning up the main function since it's doing a lot directly (many things could stand to be broken out into more functions). However, it's not that big of a deal so I'm happy to make the change.
|
Sorry for not getting back to you sooner. There's still one unaddressed review comment ( |
|
I'm a little swamped right now with work so I don't know when I'll be able to make that change. |
|
I am interested in this feature. Anything I can do to help get this merged? |
3 participants
A single
-directoryflag assumes a specific project structure wherein the app entry point and all of its dependencies are accessible in a single directory tree. However, this is not always the case.Consider mono-repos with multiple applications. Often, there are directories for each application and shared dependencies are in a different directory tree along side those applications. Additionally, applications may be categorized (for example backend, frontend, infrastructure, etc), which exacerbates the need for more flexibility in defining watchable directories.
Allowing multiple
-directoryflags maintains the existing functionality and interface while also supporting different project organizations without having to create complicated and harder to maintain-exclude-dirconfigurations.