feat: allow configuring `NewDecoder` via callbacks

[lithammer]

Since GH-59 was rejected on the basis that it wasn't useful enough to break the public API. I took a stab at an alternate solution that doesn't break the public API. While still allowing one to configure the encoder and decoder when defining it as a package global (as recommended by the README).

var decoder = NewDecoder(WithIgnoreUnknownKeysDecoderOpt(true))

Also partially solves GH-93.

[zak905]

Maybe worth keeping both functions: the original one with no parameters, and a new one called something like NewDecoderWithOptions.

Concerning the options, why there is an array of functions ? I think one can set all the options no ?

NewDecoder(func(d *Decoder)) {
  d.SetAliasTag("")
  d.ZeroEmpty(true)
   //...etc
}

In this case we will not be gaining much.

I think your approach would helpful, if there is already predefined options ready to use:

var WithAliasTagOpt = func(aliasTag string) func(d *Decoder) {
    return func(d *Decoder) {
        d.SetAliasTag(aliasTag)
    }
}

var WithIgnoreIgnoreUnknownKeysOpt = func(ignoreUnknownKeys bool) func(d *Decoder) {
    return func(d *Decoder) {
        d.IgnoreUnknownKeys(ignoreUnknownKeys)
    }
}

// ..etc an opt for each Setter function

In this case, having something like this would be more elegant:

decoder := NewDecoder(WithAliasTagOpt("alias"), WithIgnoreIgnoreUnknownKeysOpt(false))

Also how about the Encoder struct, we should do the same for NewEncoder ?

Maybe worth having a second opinion.

[lithammer]

Maybe worth keeping both functions: the original one with no parameters, and a new one called something like NewDecoderWithOptions.

Yeah I'm just a bit unsure how to design the API for that to be able to add new options without breaking backwards compatibility 🤔 You could of course apply the same strategy with the callbacks. But I feel like there must be a nicer approach available when you can start from scratch....

Concerning the options, why there is an array of functions ? I think one can set all the options no ?

NewDecoder(func(d *Decoder)) {
  d.SetAliasTag("")
  d.ZeroEmpty(true)
   //...etc
}

In this case we will not be gaining much.

Purely to be able to release it as a non-breaking change since one could still pass in zero arguments to get the old behaviour.

I think your approach would helpful, if there is already predefined options ready to use:

var WithAliasTagOpt = func(aliasTag string) func(d *Decoder) {
  return func(d *Decoder) {
      d.SetAliasTag(aliasTag)
  }
}

var WithIgnoreIgnoreUnknownKeysOpt = func(ignoreUnknownKeys bool) func(d *Decoder) {
  return func(d *Decoder) {
      d.IgnoreUnknownKeys(ignoreUnknownKeys)
  }
}

// ..etc an opt for each Setter function

In this case, having something like this would be more elegant:

decoder := NewDecoder(WithAliasTagOpt("alias"), WithIgnoreIgnoreUnknownKeysOpt(false))

Hmm yeah I guess that would look a bit neater. I guess it could be implemented separately though since the function signature would remain the same.

Also how about the Encoder struct, we should do the same for NewEncoder ?

Most likely! To be honest, I didn't consider it because decoding was my real world use-case.

Maybe worth having a second opinion.

Indeed 🙂

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.23%. Comparing base (1ab5d82) to head (550ea33). Report is 10 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #193 +/- ## ========================================== + Coverage 86.59% 87.23% +0.63% ========================================== Files 4 4 Lines 843 885 +42 ========================================== + Hits 730 772 +42 Misses 96 96 Partials 17 17 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jaitaiwan]

I've finally had the time to look at this PR. I'd agree with @zak905's suggestion of the WithAliasTagOpt etc. functions. I think if this PR included that method of doing things, with perhaps a new New function to handle these option functions as well as making the changes consistent across the Encoder struct, I think we'd be well placed to accept the PR.

Let me know your thoughts? Thanks for the effort!

[lithammer]

A problem I noticed now when trying to make it consistent with the Encoder struct is that there's going to be a naming conflict because both want similar functions:

// encoder.go
func WithAliasTag(tag string) func(d *Encoder) {
    return func(d *Encoder) {
        d.SetAliasTag(tag)
    }
}

// decoder.go
func WithAliasTag(tag string) func(d *Decoder) { // <- Error! Already defined
    return func(d *Decoder) {
        d.SetAliasTag(tag)
    }
}

[lithammer]

Guess I could do something like With{Encoder,Decoder}AliasTag, but feels a bit verbose 🤷‍♂️

[zak905]

Hi @lithammer,

What do you think about:

type Opt[T Decoder | Encoder] func(d *T)

var WithAliasDecoderOpt = func(tag string) Opt[Decoder] {
    return func(d *Decoder) {
        d.SetAliasTag(tag)
    }
}

var WithIgnoreUnkownKeysDecoderOpt = func(ignore bool) Opt[Decoder] {
    return func(d *Decoder) {
        d.IgnoreUnknownKeys(ignore)
    }
}

var WithZeroEmptyDecoderOpt = func(zeroEmpty bool) Opt[Decoder] {
    return func(d *Decoder) {
        d.ZeroEmpty(zeroEmpty)
    }
}

var WithAliasEncoderOpt = func(tag string) Opt[Encoder] {
    return func(e *Encoder) {
        e.SetAliasTag(tag)
    }
}

The definitions of the constructor functions would be:

func NewDecoder(options ...Opt[Decoder]) *Decoder {
    d := &Decoder{cache: newCache()}
    for _, opt := range options {
        opt(d)
    }
    return d
}

func NewEncoder(options ...Opt[Encoder]) *Encoder {
    e := &Encoder{cache: newCache(), regenc: make(map[reflect.Type]encoderFunc)}
        for _, opt := range options {
        opt(e)
    }
    return e
}

[jaitaiwan]

That seems pretty smooth, not going to lie

[zak905]

Hi @lithammer, are you intending to provide an implementation ? other issues like #203 may also find this useful ( for adding a new toggle flag). When introducing your changes, I think we can also eventually remove the setter functions like IgnoreUnknownKeys, ZeroEmpty...etc to make the decoder/encoder config immutable.

[lithammer]

My plan was to provide an implementation. But I'm currently on parental leave so I haven't really gotten around to it 😅 I still plan to do it, but if someone beats me to it, that's fine.

[zak905]

Allright then, enjoy the time off. Looking forward to seeing the implementation when you are back

[lithammer]

Sorry it's taken a while, but I've made an attempt at updating the feature with the above suggestions. The function names do feel a bit long though... 🤷‍♂️

[zak905]

Welcome back. I left couple of remarks. Looks descent.

[jaitaiwan]

Welcome back indeed. One small thing, I don’t see any reason to prevent folks creating their own option funcs.

[lithammer]

One small thing, I don’t see any reason to prevent folks creating their own option funcs.

I didn't see a reason to allow to create them either. So opted to be a bit conservative/defensive until maybe someone requests it. I don't have a strong opinion though.

[lithammer]

Pushed some changes. Let me know what you think.

[jaitaiwan]

LGTM as well

[lithammer]

Anything else I need to do? There's some failing security scan but that seems to be unrelated (and a false positive as well).

[jaitaiwan]

I just need to figure out an appropriate release and then merge it at the right time. This PR seems complete to me