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]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

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

☔ View full report in Codecov by Sentry.
📢 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.

[zak905]

LGTM. Thanks

[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