search

uber-go / fx

A dependency injection based application framework for Go.
https://uber-go.github.io/fx/
MIT License
5.68k stars 287 forks source link

Config using camelCase instead of underscore #256

Closed bufdev closed 7 years ago

bufdev commented 7 years ago

For yaml files, it feels more natural to have:

logging:
  text_formatter: true

Instead of:

logging:
  textFormatter: true

With how the config reads yaml files right now, the latter will work, while the former will not, unless you have explicit yaml tags (right?). Could we make this magical to do either/or? I haven't looked into it at all, so it may take a change to the yaml library.

Also something to note that you might want to use: https://github.com/peter-edge/pkg-go/blob/master/yaml/pkgyaml.go#L55

What this allows me to do is to have either yaml or json files, and do the same thing. This also makes parsing work if there are only json tags without yaml tags, which gets REALLY useful when you have protobuf/thrift entities like

message Config {
  bool stdout = 1;
}

And you can just have protobuf/thrift generate the structs, which (at least for protobuf) have json tags already, and then read them in using that function.

ascandella commented 7 years ago

cc @breerly @anuptalwalkar

HelloGrayson commented 7 years ago

FWIW, YARPC needs to support - in keys that populate a map[string]*.

For example, see some-service below:

rpc:
  outbounds:
    some-service:
      tchannel:
        with: yohoho
dmcaulay commented 7 years ago

i'm pro underscore. we also ran into issues with dashes (-) in service names.

HelloGrayson commented 7 years ago

YARPC doesn't allow capitals in service names, read: https://github.com/yarpc/yarpc-go/issues/313

ascandella commented 7 years ago

this isn't about underscores though. this is camelCase versus snake_case? my vote, as a gopher, is snakeCase, since underscore_case is basically a python-ism

dmcaulay commented 7 years ago

I've been writing too much snake_case. It looks more natural in a YAML file. I'm fine with camel though. I think it's worth picking one and sticking to it.

Did we already settle on camelCase for metric tags and values?

bufdev commented 7 years ago

Just two cents: I don't think or underscore as python, I think of it as json/yaml, ie I feel like the keys aren't linked to the programming language we are using, but that json/yaml uses underscore in general.

I'm waiting for a kubernetes user to come here and disagree with that though... :) On Thu, Feb 16, 2017 at 6:19 PM Dan McAulay notifications@github.com wrote:

I've been writing too much snake_case. It looks more natural in a YAML file. I'm fine with camel though. I think it's worth picking one and sticking to it.

Did we already settle on camelCase for metric tags and values?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/uber-go/fx/issues/256#issuecomment-280397018, or mute the thread https://github.com/notifications/unsubscribe-auth/AECGvP-XHyzZQAUpnAPvInaxPLnyyxrtks5rdIUzgaJpZM4MAbz8 .

dmcaulay commented 7 years ago

I see UberFx engdocs with snake_case 😄

ascandella commented 7 years ago

Ok, let's do snake_case then. However, we'll need to make sure that any existing structs we're populating (e.g. from internal libraries) don't have yaml annotations that force camelCase. Sound good?

bufdev commented 7 years ago

yaml annotations should be respected I think, right? Ie we should let configs specify a yaml/json annotation On Fri, Feb 17, 2017 at 11:33 AM Aiden Scandella notifications@github.com wrote:

Ok, let's do snake_case then. However, we'll need to make sure that any existing structs we're populating (e.g. from internal libraries) don't have yaml annotations that force camelCase. Sound good?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/uber-go/fx/issues/256#issuecomment-280615042, or mute the thread https://github.com/notifications/unsubscribe-auth/AECGvOdp7v04OKGsBT1cELHfGQSuzyZtks5rdXeNgaJpZM4MAbz8 .

ascandella commented 7 years ago

I just mean it will be annoying if we integrate with components that have camelCased yaml tags but the rest of our stuff is snake_cased

On Fri, Feb 17, 2017 at 2:47 AM Peter Edge notifications@github.com wrote:

yaml annotations should be respected I think, right? Ie we should let configs specify a yaml/json annotation On Fri, Feb 17, 2017 at 11:33 AM Aiden Scandella <notifications@github.com

wrote:

Ok, let's do snake_case then. However, we'll need to make sure that any existing structs we're populating (e.g. from internal libraries) don't have yaml annotations that force camelCase. Sound good?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/uber-go/fx/issues/256#issuecomment-280615042, or mute the thread < https://github.com/notifications/unsubscribe-auth/AECGvOdp7v04OKGsBT1cELHfGQSuzyZtks5rdXeNgaJpZM4MAbz8

.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/uber-go/fx/issues/256#issuecomment-280618037, or mute the thread https://github.com/notifications/unsubscribe-auth/AAF7PzFOzVxD87QkNYt-B-7l30EEZkabks5rdXragaJpZM4MAbz8 .

dmcaulay commented 7 years ago

I agree. Mixing is annoying. If using snake is going against most go packages, then it's not worth it.

akshayjshah commented 7 years ago

We had a long-ish discussion about this in the relevant zap PR. Seems like most companies end up porting the convention of their primary language to their JSON casing; in general, though, large companies do both. By way of data points:

I don't have a strong opinion, by my slight preference is to make Go packages use Go's case convention. These aren't cross-language APIs.

bufdev commented 7 years ago

Note that protobuf (out of google) uses underscore for their json tags https://github.com/peter-edge/protoeasy-go/blob/master/protoeasy.pb.go#L84

I know they are using camel case for kubernetes but I just like underscore more.

dmcaulay commented 7 years ago

@akshayjshah did you guys resolve it for zap? if so what did you decide? might as well go with that.

akshayjshah commented 7 years ago

Yep, we went with camel-casing.

bufdev commented 7 years ago

:( On Fri, Feb 17, 2017 at 9:45 PM Akshay Shah notifications@github.com wrote:

Yep, we went with camel-casing.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/uber-go/fx/issues/256#issuecomment-280761735, or mute the thread https://github.com/notifications/unsubscribe-auth/AECGvFm9jjceH_1yARh8EmBf8z1MYGt_ks5rdgb5gaJpZM4MAbz8 .

bufdev commented 7 years ago

I think camel casing wins now, closing this.