Enhancement: `form.Marshaler` and `form.Unmarshaler` #63

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Open

tigh-latte wants to merge 13 commits into go-playground:master

from tigh-latte:enhancement/custom-marshaller

Open

Enhancement: `form.Marshaler` and `form.Unmarshaler` #63

tigh-latte wants to merge 13 commits into go-playground:master from tigh-latte:enhancement/custom-marshaller

Conversation

@tigh-latte

Copy link

@tigh-latte tigh-latte commented May 9, 2024

Fixes Or Enhances

Add support for custom marshalling and unmarshalling to a struct.

The motivation behind this mainly comes from using generics, when looking to register a custom encoder/decoder, you have to register one for every generic implementation of a struct, so for example:

type MyGenericStruct[T any] struct {
 T T
}
decoder := form.NewDecoder()
decoder.RegisterCustomTypeFunc(func(x any) ([]string, error) {
 return // values
}, MyGenericStruct[Type1]{})
decoder.RegisterCustomTypeFunc(func(x any) ([]string, error) {
 return // values
}, MyGenericStruct[Type2]{})
// etc...

This isn't really great, so instead with these marshaller interfaces we can simply do:

func (g *MyGenericStruct[T]) UnmarshalForm(ss []string) error {
 // Custom handling
 return nil
}
func (g MyGenericStruct[T]) MarshalForm() ([]string, error) {
 s := []string{}
 // build s
 return s, nil
}

Make sure that you've checked the boxes below before you submit PR:

Tests exist or have been written that cover this particular change.

@go-playground/admins

tigh-latte added 5 commits

May 9, 2024 12:00

@tigh-latte


 add FormMarshaler and FormUnmarshaler

97397eb

@tigh-latte


 encoder error tests

2d15be0

@tigh-latte


 rename marshaller interface to prevent stutter

ab25a15

@tigh-latte


 update README

ef112dc

@tigh-latte


 remove unnecessary shadow in encoder, use already define var in decoder

eab201f

@coveralls

Copy link

coveralls commented May 9, 2024 •

edited

Loading

Coverage Status

coverage: 99.646% (-0.2%) from 99.814%
when pulling b045670 on tigh-latte:enhancement/custom-marshaller
into 8785d3c on go-playground:master.

@tigh-latte

Copy link

Author

tigh-latte commented May 9, 2024

Looks like gofumpt also made a number of formatting edits. Let me know if this is something yous don't want.

tigh-latte added 4 commits

May 9, 2024 12:58

@tigh-latte


 slice tests

e904ed6

@tigh-latte


 fix pointer test

@tigh-latte


 map encoding tests

54d6dbf

@tigh-latte


 map encoder tests, more decoder tests

bc69006

@deankarn

Copy link

Contributor

deankarn commented May 11, 2024

Thank you for the PR @tigh-latte! I think this would be an amazing addition and will take a deeper look at it this weekend.

I already took a cursory look and code looks solid, I am only thinking about the interface and its signature function/method name.

@tigh-latte

Copy link

Author

tigh-latte commented May 11, 2024

Sure thing, let me know if you have any better/preferred suggestions.

deankarn

deankarn reviewed

May 12, 2024

View reviewed changes

Copy link

Contributor

@deankarn deankarn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @tigh-latte

I was able to find time to more thoroughly review the code and think about the interface names.

I like the interface names as is :)
I left a comment to see if changing the unmarshalling logic slightly was possible only to keep the flow of the program using the existing Ptr logic.

And again TY for the PR, can't wait to merge this.

encoder.go Outdated

return

}

{

Copy link

Contributor

@deankarn deankarn May 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure the extra scope is necessary.

Copy link

Author

@tigh-latte tigh-latte May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not

encoder.go Outdated

Comment on lines 102 to 104

if t := v.Type(); t.Kind() == reflect.Ptr && v.IsNil() {

return

}

Copy link

Contributor

@deankarn deankarn May 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this check necessary?

Below this logic reflect.Ptr is handled and returned.
What is someone wanted to set a default value when it was nil from the custom marshaller, would this prevent that?

Copy link

Author

@tigh-latte tigh-latte May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I've played around with this, and could get something sorted that didn't risk a nil pointer in some form.

For example, by removing this check and making one or two other modifications, I ended up allowing this following to work:

func (c *custom) MarshalForm() ([]string, error) {
 if c == nil {
 return // some default value
 }
 return // real handling
}
func main() {
 var c *custom
 NewEncoder().Encode(c)
}

However, if MarshalForm is changed to have a value receiver, then we'd end up with a nil panic because the nil *custom passes the form.Marshaler implementation check, then have its MarshalForm function called, causing the panic once dereferenced to the receiver.

All this playing about ended up having me take a read of the json.Marshaler implementation in the standard library. It's behaviour is basically just, if the item being marshalled is nil, write "null" (see here, and here).

The way the std lib handles its reflection checks is much better than what I've done here so I'm gonna do a small rewrite, but do let me know what behaviour you would prefer regarding this.

decoder.go Outdated

}

{

Copy link

Contributor

@deankarn deankarn May 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure the extra scope is needed.

Copy link

Author

@tigh-latte tigh-latte May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added so that v could be shadowed and modified without changing the code later on, but I'll just assign all modifications to a new var instead.

form_decoder.go Outdated

// Unmarshaler is the interface implemented by an object that can unmarshal a form representation of itself.

type Unmarshaler interface {

UnmarshalForm(ss []string) error

Copy link

Contributor

@deankarn deankarn May 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

UnmarshalForm(ss[]string) error

UnmarshalForm([]string) error

decoder.go Outdated

Comment on lines 198 to 209

v := v // deliberately shadow v as to not modify

t := v.Type()

if t.Kind() != reflect.Ptr && v.CanAddr() {

v = v.Addr()

}

if um, ok := v.Interface().(Unmarshaler); ok {

// if receiver is a nil pointer, set before calling function.

if t.Kind() == reflect.Ptr && v.IsNil() {

t = t.Elem()

v.Set(reflect.New(t))

um = v.Interface().(Unmarshaler)

}

Copy link

Contributor

@deankarn deankarn May 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to avoid doing this/duplicating the Ptr logic of creating the new element?

below this same logic exists for Ptr already https://github.com/go-playground/form/pull/63/files#diff-2a967f84f1af0d52111b8dbfbeadc44bfeb1092dacebb2bc0648a0e57284c04bR226-R230.

It would be great if it's possible to keep the flow of the program using that same logic.

Copy link

Author

@tigh-latte tigh-latte May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a look at this and see if anything can be done

tigh-latte added 4 commits

May 13, 2024 15:55

@tigh-latte


 standard lib ideas, tidier handling

58d5d48

@tigh-latte


 have the encoder marshal funcs return an error

8eda55b

@tigh-latte


 use legacy functions

98b9046

@tigh-latte


 found, fixed, and tested a nil panic

b045670

Labels

None yet

3 participants

@tigh-latte @coveralls @deankarn

Enhancement: form.Marshaler and form.Unmarshaler #63

Are you sure you want to change the base?

Enhancement: form.Marshaler and form.Unmarshaler #63

Uh oh!

Conversation

@tigh-latte tigh-latte commented May 9, 2024

Fixes Or Enhances

Uh oh!

coveralls commented May 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tigh-latte commented May 9, 2024

Uh oh!

deankarn commented May 11, 2024

Uh oh!

tigh-latte commented May 11, 2024

Uh oh!

@deankarn deankarn left a comment

Choose a reason for hiding this comment

Uh oh!

@deankarn deankarn May 12, 2024

Choose a reason for hiding this comment

Uh oh!

@tigh-latte tigh-latte May 13, 2024

Choose a reason for hiding this comment

Uh oh!

@deankarn deankarn May 12, 2024

Choose a reason for hiding this comment

Uh oh!

@tigh-latte tigh-latte May 13, 2024

Choose a reason for hiding this comment

Uh oh!

@deankarn deankarn May 12, 2024

Choose a reason for hiding this comment

Uh oh!

@tigh-latte tigh-latte May 13, 2024

Choose a reason for hiding this comment

Uh oh!

@deankarn deankarn May 12, 2024

Choose a reason for hiding this comment

Uh oh!

@deankarn deankarn May 12, 2024

Choose a reason for hiding this comment

Uh oh!

@tigh-latte tigh-latte May 13, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Enhancement: `form.Marshaler` and `form.Unmarshaler` #63

Enhancement: `form.Marshaler` and `form.Unmarshaler` #63

coveralls commented May 9, 2024 •

edited

Loading