Delete multiple file specification and dir-mode #161
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #161 +/- ##
==========================================
+ Coverage 67.64% 67.93% +0.28%
==========================================
Files 73 73
Lines 3799 3739 -60
==========================================
- Hits 2570 2540 -30
+ Misses 891 868 -23
+ Partials 338 331 -7
Continue to review full report at Codecov.
|
| @@ -895,7 +893,7 @@ func testDoInternal(stdin io.Reader, args ...string) (string, int) { | |||
| } | |||
| buffer := bytes.NewBuffer(nil) | |||
| // develMode is on, so we have access to all commands | |||
| exitCode := do(true, args, stdin, buffer, os.Stderr) | |||
| exitCode := do(true, args, stdin, buffer, buffer) | |||
There was a problem hiding this comment.
We want to use the same buffer for both stdout and stderr? Seems fine in a test - just want to make sure that's what you intend.
There was a problem hiding this comment.
Yes, I took this out before, but as the tests operate on the output, I want to check that ALL output is as I expect (which is generally that stderr has no output)
There was a problem hiding this comment.
Cool, thanks for clarifying!
| flags.bindDryRun(flagSet) | ||
| }, | ||
| } | ||
|
|
||
| grpcCmdTemplate = &cmdTemplate{ | ||
| Use: "grpc dirOrProtoFiles...", | ||
| Use: "grpc [dirOrFile]", |
There was a problem hiding this comment.
Should we add cobra.MaximumNArgs(1) here, too?
There was a problem hiding this comment.
It's already there, it's below the Long documentation
There was a problem hiding this comment.
Doh! Sorry for missing that earlier.
internal/exec/runner.go
Outdated
| if len(args) == 0 { | ||
| // TODO: does not fit in with workDirPath paradigm | ||
| args = []string{"."} | ||
| func (r *runner) getMeta(args []string, lenOfArgsIfSpecified int) (*meta, error) { |
There was a problem hiding this comment.
lenOfArgsIfSpecified might be better represented as maxArgsLength (or something similar). I think this would make the following conditional(s) more clear. What are your thoughts?
There was a problem hiding this comment.
This is the length of the args if a file or dir was specified, it’s not max args per se. It says “if args are length 3, then we have a dir or file, otherwise use pwd”. A comment could help, I’ll add one
There was a problem hiding this comment.
Cool, that sounds good. Thanks!
There was a problem hiding this comment.
LGTM, same nit as @amckinney lenOfArgsIfSpecified took me a bit to understand, comment/rename would be great.
CHANGELOG.md
Outdated
| @@ -5,7 +5,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) | |||
| and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). | |||
|
|
|||
| ## [Unreleased] | |||
| - No changes yet. | |||
| - Delete the ability to explicitly specify multiple files, and have the effect | |||
There was a problem hiding this comment.
should be under a ### Removed header
There was a problem hiding this comment.
It's really Changed, since it's remove multiple files, have single file be old dir mode.
|
All comments addressed @AlexAPB @amckinney |
| flags.bindDryRun(flagSet) | ||
| }, | ||
| } | ||
|
|
||
| grpcCmdTemplate = &cmdTemplate{ | ||
| Use: "grpc dirOrProtoFiles...", | ||
| Use: "grpc [dirOrFile]", |
There was a problem hiding this comment.
Doh! Sorry for missing that earlier.
| // contains the dirOrFile parameter as it's first argument." For example, for | ||
| // "prototool compile [dirOrFile]", lenOfArgsIfSpecified is 1, saying that if | ||
| // we have 1 argument, the first argument is dirOrFile, if we have zero arguments, | ||
| // we do not and assume the current directory is the dirOrFile. |
There was a problem hiding this comment.
Thanks for the additional note!
Fixes #16. The majority of the diff is splitting up files used in integration testing into individual directories so that the tests still work. Since all
prototoolbuilds now operate on directories, this is required.