Use of bools in function parameters

Hello,

I am looking at configstate.Configure function:

func Configure(s *state.State, snapName string, patch map[string]interface{}, flags int) *state.TaskSet

Is there any reason to have flags:int instead of two bools?

func Configure(s *state.State, snapName string, patch map[string]interface{}, ignoreError bool, trackError bool) *state.TaskSet

To me bool version is much more cleaner.

There’s actually a good reason for that: bools in that context hinder code readability:

    Breakfast(true)

vs.

    Breakfast(NeedsCoffee)
2 Likes

I guess in go there is no named params, so the closest thing would be:

needsCoffee = true
Breakfast(needsCoffee)

Setting and getting bitmasks are not nicer in my opinion.
My problem with this function was also that to get booleans out if bitmasks code has to reference snapstate package, which is really overkill. In my branch I cannot reference snapstate as it would introduce cyclic dependency.
Probably I will have to live with it for some time more untill pre-start hook is introduced.

I don’t think there’s a lot to argue about regarding which version of the code in the example above was more clear. This is really the background of the choice.

If you have specific cycles you’d like to see solved, that’s an independent issue. Please just open a new topic for it and we’ll work with you to get it sorted.

I was talking about this function:

func Configure(s *state.State, snapName string, patch map[string]interface{}, flags int) 
    ...
    IgnoreError: flags&snapstate.IgnoreHookError != 0,
    TrackError:  flags&snapstate.TrackHookError != 0,
    ...
}
var confFlags int
if snapsup.Name() == "core" {
	confFlags |= IgnoreHookError
	confFlags |= TrackHookError
}
configSet := Configure(st, snapsup.Name(), defaults, confFlags)

Why adding dependency back to snapstate to later resolve bools instead of resolving them right before the call?

I am a bit sensitive to this snapstate dependency here as in my version of snapd I am maintaining pre-start hook and for that I have removed late binding of Configure function and instead added dependency on configstate for simpler implementation.

FWIW we’re moving away from flags-as-bitmask to flags-as-struct (see overlord/snapstate/flags.go for example); but it’s relatively low priority.