Skip to content
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

Add support for encoding.TextUnmarshaler #6

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

qexpres
Copy link

@qexpres qexpres commented Apr 14, 2020

I thought I would give your request a chance. Adding types that implement the encoding.TextUnmarshaler interface should work now.

closes #1

@qexpres
Copy link
Author

qexpres commented Apr 15, 2020

I'm sorry for the forced push. I kinda hoped that the failed checks would be a temporary problem so I tried to retrigger them. Not sure what's wrong, though. I didn't change anything related to the integration tests.

@ianlopshire
Copy link
Owner

@qexpres,

Thanks for this PR! Everything looked good during the quick once-over I just did. I'll take a look at the integration tests and do a full review this evening or sometime this weekend.

Thanks again!

@ianlopshire ianlopshire self-requested a review April 17, 2020 16:04
@qexpres
Copy link
Author

qexpres commented Apr 21, 2020

@ianlopshire, no problem, take your time. I can imagine that you have enough things on your mind during these crazy times anyway. Let me know if I can help.

Copy link
Owner

@ianlopshire ianlopshire left a comment

Choose a reason for hiding this comment

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

@qexpres,

I apologize for the delay!

Overall this looks pretty solid, but there are a couple of changes I'd like to see before merging (see code comments).

Thanks again!

ssmconfig.go Outdated

// Create a new instance of the field's type and call its UnmarshalText([]byte) method.
// Set the value after execution and fail if the method returns an error.
func unmarshalText(f reflect.StructField, v reflect.Value, s string) error {
Copy link
Owner

@ianlopshire ianlopshire May 1, 2020

Choose a reason for hiding this comment

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

This can be simplified quite a bit. The Interface() method on reflect.Value allows you to access the value as a regular go interface.

We also need to handle the case where the value is already a pointer to the implementation of TextUnmarshaled. I would handle this by accepting a shouldAddr argument.

My implementation of this would probably look something like:

func unmarshalText(t reflect.Type, v reflect.Value, s string, shouldAddr bool) error {
	if shouldAddr {
		v = v.Addr()
	}
	if t.Kind() == reflect.Ptr && v.IsNil() {
		v.Set(reflect.New(t.Elem()))
	}
	return v.Interface().(encoding.TextUnmarshaler).UnmarshalText([]byte(s))
}

Copy link
Author

Choose a reason for hiding this comment

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

I will definitely change the much simpler method invocation, thanks!

I didn't add support for pointers on purpose as all the other types don't support pointers yet. I thought this could be addressed in a new issue and possibly implemented using a more dynamic / recursive approach for all supported types.

What do you think? Should I add pointer support already or not?

Copy link
Owner

Choose a reason for hiding this comment

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

Hmmmmmm, Good point.

I think I would go ahead and add pointer support for TextUnmarshaler. It starts to get confusing what is and isn't supported when you don't allow pointers for interface types.

Copy link
Author

@qexpres qexpres May 4, 2020

Choose a reason for hiding this comment

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

✔️

I also added tests for empty non-required parameters and added another example to the readme.

ssmconfig.go Outdated
Comment on lines 87 to 100
err = setValue(v.Field(i), value)
valueField := v.Field(i)
typeField := t.Field(i)
var err error
if isTextUnmarshaler(typeField) {
err = unmarshalText(typeField, valueField, value)
} else {
err = setValue(valueField, value)
}
Copy link
Owner

@ianlopshire ianlopshire May 1, 2020

Choose a reason for hiding this comment

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

I would move all of the logic for TextUnmarshaler into the setValue function.

We also need to handle the case where the value is already a pointer to the implementation of TextUnmarshaled.

Something like:

func setValue(v reflect.Value, s string) error {
	t := v.Type()

	if t.Implements(textUnmarshalerType) {
		return unmarshalText(t, v, s, /*shouldAddr*/ false)
	}
	if reflect.PtrTo(t).Implements(textUnmarshalerType) {
		return unmarshalText(t, v, s, /*shouldAddr*/ true)
	}
	
	//...
}

@qexpres
Copy link
Author

qexpres commented Jun 16, 2020

Hey, trying to not sound impatient, but do you think this PR is ready for merging? Or is there still something you need me to do?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for encoding.TextUnmarshaler
2 participants