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

Config using camelCase instead of underscore #256

Closed
bufdev opened this issue Feb 14, 2017 · 18 comments
Closed

Config using camelCase instead of underscore #256

bufdev opened this issue Feb 14, 2017 · 18 comments

Comments

@bufdev
Copy link
Contributor

bufdev commented Feb 14, 2017

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
Copy link
Contributor

cc @breerly @anuptalwalkar

@HelloGrayson
Copy link
Contributor

HelloGrayson commented Feb 14, 2017

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
Copy link
Contributor

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

@HelloGrayson
Copy link
Contributor

YARPC doesn't allow capitals in service names, read: yarpc/yarpc-go#313

@ascandella
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

bufdev commented Feb 16, 2017 via email

@dmcaulay
Copy link
Contributor

I see UberFx engdocs with snake_case 😄

@ascandella
Copy link
Contributor

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
Copy link
Contributor Author

bufdev commented Feb 17, 2017 via email

@ascandella
Copy link
Contributor

ascandella commented Feb 17, 2017 via email

@dmcaulay
Copy link
Contributor

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

@akshayjshah
Copy link
Contributor

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:

  • Twitter's API is snake cased.
  • Facebook's API is snake cased.
  • AWS is inconsistent.
  • Google is mostly camel cased, and their JSON style guide recommends camel.

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
Copy link
Contributor Author

bufdev commented Feb 17, 2017

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
Copy link
Contributor

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

@akshayjshah
Copy link
Contributor

Yep, we went with camel-casing.

@bufdev
Copy link
Contributor Author

bufdev commented Feb 17, 2017 via email

@bufdev
Copy link
Contributor Author

bufdev commented Mar 6, 2017

I think camel casing wins now, closing this.

@bufdev bufdev closed this as completed Mar 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants