gobmp
gobmp copied to clipboard
Simplify argument handling
@taktv6 Thank you for PR, I am curious what would be the advantage of using proposed argument handing approach. I saw in the past both approaches were used interchangeably, so unless there is a good technical/practical reason/improvement, I do not see a need to change just for the change sake. Please share your reasoning. Thank you in advance.
This is the first repo I find another approach in. There is just no gain in splitting variable declaration and flag.* calls. Also my PR avoids the parsing of strings to get boolean parameters. Flag package is designed to handle this and actually handles that very well.
@taktv6 Unfortunately handling bool variable is broken in kubernetes pod's yaml definition. In order to provide consistent bool processing in k8s and in other environments, we had to use string and then convert it into bool.
Give the following example:
import (
"flag"
"fmt"
)
var (
someFlag = flag.Bool("someFlag", false, "Foo Bar")
)
func main() {
flag.Parse()
fmt.Printf("someFlag: %v\n", *someFlag)
}
takt@tl1 ~/git/src/github.com/taktv6/flagtest % ./flagtest
someFlag: false
takt@tl1 ~/git/src/github.com/taktv6/flagtest % ./flagtest -someFlag=true
someFlag: true
takt@tl1 ~/git/src/github.com/taktv6/flagtest % ./flagtest -someFlag=false
someFlag: false
takt@tl1 ~/git/src/github.com/taktv6/flagtest % ./flagtest -someFlag
someFlag: true
While this is probably expected I guess you mean this:
takt@tl1 ~/git/src/github.com/taktv6/flagtest % ./flagtest -someFlag true
someFlag: true
takt@tl1 ~/git/src/github.com/taktv6/flagtest % ./flagtest -someFlag false
someFlag: true
However, you're free to do this, no?
spec:
template:
spec:
containers:
- args:
- -someFlag=true
Instead of:
spec:
template:
spec:
containers:
- args:
- -someFlag
- true
Or am I missing anything?
One more thought about the current bool flag handling. With the current implementation enabling split AF using just
./gobmp -split-af
does not work. However I would expect it to work as it is a boolean flag.
-l
is a boolean parameter to ls
. We're expecting ls -l
to work and not to require ls -l true
, right?