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

Merge {marshal,unmarshal}TableValue into marshalValue. #1393

Merged
merged 1 commit into from
Jun 18, 2015

Conversation

petermattis
Copy link
Collaborator

This allows easy setting of "typed" values with the standard key/value
methods which will be used shortly for setting and retrieving the
"descriptor ID" value for namespace keys.

This allows easy setting of "typed" values with the standard key/value
methods which will be used shortly for setting and retrieving the
"descriptor ID" value for namespace keys.
@petermattis petermattis force-pushed the pmattis/client-unmarshal branch from c402ced to b70fa1d Compare June 16, 2015 20:04
return nil

case *gogoproto.Message:
panic("TODO(pmattis): unimplemented")
Copy link
Member

Choose a reason for hiding this comment

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

seems awkwardish to panic when you could return an error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

An error might be ignored or logged. I wanted something more forceful to as a reminder to implement this support. Should be trivial to do, just haven't done it yet.

@tbg
Copy link
Member

tbg commented Jun 17, 2015

LGTM. It's a bit hard to embrace the copious amounts of reflection since we've always tried to avoid it wherever possible, but this being a client library, us having decided to go down this road, and the simplification in this case being significant, this is certainly the right thing to do.

@petermattis
Copy link
Collaborator Author

FWIW, the database/sql package has similar amounts of reflection internally in order to deal with type conversions in the {Row,Rows}.Scan() method. The other bits of reflection to walk over the fields of structs is pretty much required for a Go ORM. The reflection certainly complicates the implementation, but I think it leaves the API usable. I'm very open to alternatives that are both reasonably clean and similarly performant.

@tbg
Copy link
Member

tbg commented Jun 18, 2015

No need to convince me, I'm on your side.

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.

2 participants