Allow only one prototool.yaml file #139
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #139 +/- ##
==========================================
- Coverage 68.06% 67.84% -0.23%
==========================================
Files 72 72
Lines 3786 3800 +14
==========================================
+ Hits 2577 2578 +1
- Misses 866 878 +12
- Partials 343 344 +1
Continue to review full report at Codecov.
|
| ``` | ||
|
|
||
| Everything under `a/d` will use `a/d/prototool.yaml`, everything under `b`, `b/g/h` will use `b/prototool.yaml`, and everything under `a`, `a/e`, `a/f`, `c`, `c/i` will use `prototool.yaml`. See [internal/file/testdata](internal/file/testdata) for the most current example. | ||
| If multiple `prototool.yaml` files are found that match the input directory or files, an error will be returned. We have an ongoing discussion about whether to allow multiple `prototool.yaml` files, see [this issue](https://github.com/uber/prototool/issues/10) for more details. |
There was a problem hiding this comment.
What does it mean for prototool.yaml files to match the input directory or files?
Seeing as you cannot have more than one prototool.yaml file in a given directory, could we simply just use the prototool.yaml file that corresponds to where the command was run, and ignore all others? For example, in your removed tree example, running prototool <cmd> in b/g should just use the prototool.yaml found there (and not the one included in the root). Similarly, running the same command at the root would use the root prototool.yaml and ignore all others.
Let me know if this isn't what you meant - just trying to determine when an error would occur and whether or not it's necessary.
| func (c *compiler) ProtocCommands(protoSet *file.ProtoSet) ([]string, error) { | ||
| // we end up calling the logic that creates temporary files for file descriptor sets | ||
| // anyways, so we need to clean them up with cleanCmdMetas | ||
| // this logic could be simplified to have a "dry run" option, but ProtocCommands |
There was a problem hiding this comment.
nit: Now that this uses the dry-run flag, could we remove this comment? My fault for not handling in #135
There was a problem hiding this comment.
Can't remove this yet, needs some refactoring, this still creates temporary files.
internal/file/proto_set_provider.go
Outdated
| for _, protoSet := range protoSets { | ||
| configDirPaths = append(configDirPaths, protoSet.Config.DirPath) | ||
| } | ||
| return nil, fmt.Errorf("multiple configuration files found in directories %v for proto files found for dirPath %q but expected exactly one configuration file", configDirPaths, dirPath) |
There was a problem hiding this comment.
These are absolute paths if i'm not mistaken, so it may be more readable to reword the error message to have the directories listed last
eg:
return nil, fmt.Errorf("expected exactly one configuration file for dirPath %q, but found multiple in directories: %v", dirPath, configDirPaths)
internal/file/proto_set_provider.go
Outdated
| for _, protoSet := range protoSets { | ||
| configDirPaths = append(configDirPaths, protoSet.Config.DirPath) | ||
| } | ||
| return nil, fmt.Errorf("multiple configuration files found in directories %v for proto files found for filePaths %v but expected exactly one configuration file", configDirPaths, filePaths) |
Per #10 and #93, this change does the bare minimum to enforce only one
prototool.yamlfile per command. There is further refactoring that can be done after this, but this propagates the changes through the codebase while retaining the multipleprototool.yamlfile support in the implementation for now. Putting this up for discussion, it also helps simplify the logic for #131.