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 decoder when defining it as a package global (as recommended by the README).

var decoder = NewDecoder(func(d *Decoder) {
	d.IgnoreUnknownKeys(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

Merging #193 (74b3a47) into main (5fca2dc) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #193      +/-   ##
==========================================
+ Coverage   83.23%   83.33%   +0.09%     
==========================================
  Files           4        4              
  Lines         710      714       +4     
==========================================
+ Hits          591      595       +4     
  Misses        103      103              
  Partials       16       16              

Files Changed	Coverage Δ
decoder.go	76.06% <100.00%> (+0.29%)	⬆️

[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