Add type interface{} target as argument to Field.Binder?

[mark-kubacki]

As incentive to delve into this topic I've drafted up a Gist to illustrate the current state of Binding's usage in the wild. I will introduce some ideas to Binding referring to that example:
https://gist.github.com/wmark/73ee2abe30e13af93a18#file-mailgun-go-L62-L70

In order to create a library (or to add it to Binding itself) we have to add another argument to Field.Binder, by which the function can access formVals[]' destination. (Please note the suggested functions in the comments.)

Or, do you think using closures would suffice here and in other use-cases?

[mholt]

How about naming the Binder function and doing this:

// proposed revision of https://gist.github.com/wmark/73ee2abe30e13af93a18#file-mailgun-go-L62-L70
&f.Signature: binding.Field{
    Form: "signature",
    Binder: bindHexField
}

// This function is now reusable!
func bindHexField(fieldName string, formVals []string, errs binding.Errors) binding.Errors {
    messageMAC, err := hex.DecodeString(formVals[0])
    if err != nil {
        errs.Add([]string{fieldName}, binding.DeserializationError, err.Error())
    } else {
        f.Signature = messageMAC
    }
    return errs
}

Now the bindHexField is available for re-use. Is this what you had in mind? (By naming the function, any Binder function becomes be reusable.)

[mark-kubacki]

Yes, f.Signature is in the scope of ….FieldMap(), but I am afraid this won't suffice. f.Signature as found in bindHexField is no global variable and I'd like to move the functions to a package of its own (binding.fields? wmark/go-formencode?).

The two variants are, (1):

func BindHexEncodedField(field *[]byte) func(string, []string, binding.Errors) binding.Errors {
    return func(fieldName string, formVals []string, errs binding.Errors) binding.Errors {
        var err error // we cannot use := below because some versions of Golang would override the enclosed "field"
        *field, err = hex.DecodeString(formVals[0])
        if err != nil {
            errs.Add([]string{fieldName}, binding.DeserializationError, err.Error())
        }
        return errs
    }
}

…
func (f *MailgunParsedMessage) FieldMap() binding.FieldMap {
    return binding.FieldMap{
        …
        &f.Signature: binding.Field{
            Form:   "signature",
            Binder: BindHexEncodedField(&f.Signature),
        },
        …
    }
}

Obviously we should turn func(string, []string, Errors) Errors into a reusable type of its own.

Variant (2) requires modifications to Field.Binder, bindForm and is:

func HexEncodedFieldBinder(someField interface{}, fieldName string, formVals []string, errs binding.Errors) binding.Errors {
    field := someField.(*[]bytes)
    *field, err := hex.DecodeString(formVals[0])
    if err != nil {
        errs.Add([]string{fieldName}, binding.DeserializationError, err.Error())
    }
    return errs
}

…
func (f *MailgunParsedMessage) FieldMap() binding.FieldMap {
    return binding.FieldMap{
        …
        &f.Signature: binding.Field{
            Form:   "signature",
            Binder: HexEncodedFieldBinder,
        },
        …
    }
}

Re (1): Closures add some overhead and result in work for the GC. Type conversion becomes trivial, though. Requires no modifications to the package.
Re (2): Signatures of Field.Binder and Bind.Binder would diverge. Easy on memory and GC.

Which one do you like the best? Or is there another variant I missed?

[bhcleek]

I really prefer the first option. It has the advantage of not requiring changes to any code in the wild that's already using binding, as you said, type conversion is trivial, and keeps it as simple as possible.

[mark-kubacki]

Very well, I've updated the example. If Matt agrees I would like to introduce this to binding.go, because otherwise such function definitions (see the example's new fields.go)will become very long:

type FieldBinderFunc func(string, []string, Errors) Errors
// or
type BinderSignature func(string, []string, Errors) Errors

I am open to suggestions for the name. ;-)

[bhcleek]

FieldBinderFunc seems right to me.

[mholt]

I prefer the first variation.

By the way, if a line like

type BinderFunc func(string, []string, Errors) Errors

would help simplify things, I'm okay with that.

So before we go forward, can you help me understand what you're adding to the package, exactly? It doesn't look like this requires any real changes to the code, so is it some pre-defined Binder funcs that are being added?

[mark-kubacki]

Matt, I am not adding anything at without a consensus. The only thing we agreed on is the line you've mentioned above. Until I let go of this issue:

• present some benchmarks
• another look at the signature (do we need the full binding.Errors in a field-binder as input value, return value, as non-pointer…) to make it simpler, if possible

[mholt]

Okay, yeah, I'm good with adding that line. I just wasn't sure if you were also adding some other functions.

About the signature which receives binding.Errors as an argument, I think that was deemed necessary early on so that errors could be persisted throughout the validation process for an entire request (without having to resort to sessions or global variables or something).