-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Allow specifying a StringSlice destination for StringSliceFlag #1078
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1078 +/- ##
==========================================
+ Coverage 72.83% 72.91% +0.07%
==========================================
Files 33 33
Lines 2474 2481 +7
==========================================
+ Hits 1802 1809 +7
Misses 565 565
Partials 107 107 Continue to review full report at Codecov.
|
I've found a bug in this code so far. The problem is that when you provide both environment variables and flags, you end up appending to the array instead of replacing one with the other. For example: Environment: |
I think I've solved it, but I'm going to do some more testing. |
Based on my local testing this now works as expected, flags will override the slice set by env vars entirely. |
package main
import (
"fmt"
"github.com/davidsbond/cli"
"log"
"os"
)
var (
dest = &cli.StringSlice{}
)
func main() {
os.Setenv("TEST_THIS_THING", "1,2,3")
app := cli.NewApp()
app.Action = run
app.Flags = []cli.Flag{
&cli.StringSliceFlag{
Name: "test",
Destination: dest,
EnvVars: []string{"TEST_THIS_THING"},
},
}
if err := app.Run(os.Args); err != nil {
log.Fatal(err)
}
}
func run(c *cli.Context) error {
fmt.Println(dest.Value())
return nil
}
|
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 don't have any particular opinions about the env var handling, but everything else LGTM! 🚢
What type of PR is this?
What this PR does / why we need it:
Adds a
Destination
field to theStringSliceFlag
of typeStringSlice
so that we can use a variable to access the value rather than callingc.StringSlice
. This adds some additional type safety and can help to avoid scenarios where you have typos calling thec.StringSlice("name")
method.Which issue(s) this PR fixes:
Fixes #1075
Testing
By running the code I've seen that the
Destination
gets set, I've also added in some tests for setting the destination value from env and from flagsRelease Notes