gobmp icon indicating copy to clipboard operation
gobmp copied to clipboard

Simplify argument handling

Open taktv6 opened this issue 2 years ago • 5 comments

taktv6 avatar Jun 11 '22 10:06 taktv6

@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.

sbezverk avatar Jun 11 '22 15:06 sbezverk

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 avatar Jun 11 '22 15:06 taktv6

@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.

sbezverk avatar Jun 11 '22 16:06 sbezverk

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?

taktv6 avatar Jun 13 '22 08:06 taktv6

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?

taktv6 avatar Jun 13 '22 08:06 taktv6