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

Embedded structs don't have their config merged up #22

Closed
abraithwaite opened this issue Mar 27, 2018 · 7 comments
Closed

Embedded structs don't have their config merged up #22

abraithwaite opened this issue Mar 27, 2018 · 7 comments

Comments

@abraithwaite
Copy link
Contributor

abraithwaite commented Mar 27, 2018

When processing command line flags, you must specify the embedded structs name or tag when loading configuration. This is unlike the behavior of other deserializers (json, primarily).

An example is much easier to understand than this is to explain:

abraithwaite at alan-mbpr in ~GOPATH/src/github.com/segmentio
15:58:58 $ go run play.go -name "blah" -D.location "SF"
Hello, blah
{"name": "asdf", "location": "NY"}
{"location":"SF","name":"blah"}
abraithwaite at alan-mbpr in ~GOPATH/src/github.com/segmentio
15:59:14 $ go run play.go -name "blah" -location "SF"
Usage:
  play [-h] [-help] [options...]

Options:
  -D object

  -D.location string

  -config-file source
        Location to load the configuration file from.

  -name string
        (default blah)

Error:
  flag provided but not defined: -location

exit status 1

For the file: https://gist.github.com/abraithwaite/3f74d9fbd06dbcba0c2ac14d61cc2a09#file-play-go

It would be preferable (although it would be a breaking change, maybe that can be avoided) for the properties on D to be merged up to C without needing a prefix. Perhaps we can get away with a special tag like, but not equal to -.

@achille-roussel
Copy link
Contributor

Would you be able to submit a patch?

Although there may be code that rely on the current behavior that would get broken by such a change.

Is your current workaround to copy the fields directly in the struct?

@abraithwaite
Copy link
Contributor Author

My current workaround is just to live with a prefix. It's not a major issue right now and I don't plan on working on it anytime soon, but it's low hanging fruit for anyone who wants to take a shot.

@achille-roussel
Copy link
Contributor

achille-roussel commented Mar 28, 2018

Cool.

I've been thinking about how to avoid those kinds of discrepancies. Currently we build an intermediary representation of the configuration in memory, which we then convert to the configuration struct type.

Instead of doing this we could serialize the configuration to JSON, then use the standard JSON decoder to load values into the configuration struct. This way there can be no behavior differences (this is the approach taken by packages like https://github.com/ghodss/yaml for example).

Food for thoughts!

@stevevls
Copy link
Contributor

stevevls commented May 8, 2018

In order to avoid breaking compatibility, what do you guys think about using _ as the special tag to indicate that the embedded struct shouldn't be prefixed?

I'm thinking about taking a stab at this too (my particular use case is creating and embedding general Suppressor configuration to be shared between the Kafka and NSQ apps). Happy to brainstorm further because this would be a really cool feature.

@achille-roussel
Copy link
Contributor

That sounds like a good idea to me 👍

@achille-roussel
Copy link
Contributor

Doing it backward-compatible seems like a good first step, if you want to make it the default then we can tag a v1 release of the package, and introduce the change afterward.

@abraithwaite
Copy link
Contributor Author

This was fixed in #23.

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

No branches or pull requests

3 participants