[
https://issues.apache.org/jira/browse/THRIFT-3170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14557722#comment-14557722
]
Paul Magrath commented on THRIFT-3170:
--------------------------------------
Hello!
So there are a fair few points above, some of which I agree with and some I'd
have a slightly differing opinion regarding.
1. I would consider Id to simply be a bug. Gpu is unfortunate, we could add it
to the list but we can never build an all encompassing list, it will only ever
be best effort and sticking with the list the golint guys have seems
reasonable. Yes, this accepts that there will be cases of inconsistencies but
we were only ever trying to handle the common initialisms.
2. I'm not really following your one way argument tbh. It seems to assume that
there isn't any human written code following Go best practice regarding
initialisms in the code bases at all which I wouldn't think would generally be
the case. If there are Go database libraries making such assumptions without
handing initialisms as they should be, I think that's probably a bug in them
and a bug that will remain when dealing with human written code regardless of
what Thrift does.
3. Competely agree, it is a nice to have that makes the generated code more
like human written code. It is purely aesthetic enhancement that doesn't make a
functional difference really.
4. There are many differences between those three languages, this is a fairly
minor one that mistakes regarding will generally get caught at compile time.
Regarding it being a usability issue, I think there is a balance here between
making the generated code more comfortable for novice Go programmers and making
it more comfortable for experienced Go programmers.
I think the idea to flag-ify the initialism behaviour so that people who want
to can disable it seems like a good idea. I think including the initialisms in
the default behaviour makes sense, best argument against it is the backwards
compatibility one although personally in this case I don't find that
particularly convincing in this case.
Happy to help out with the flag-ification next week. :)
> Initialism code in the Go compiler causes chaos
> -----------------------------------------------
>
> Key: THRIFT-3170
> URL: https://issues.apache.org/jira/browse/THRIFT-3170
> Project: Thrift
> Issue Type: Bug
> Components: Go - Compiler
> Affects Versions: 0.9.2
> Environment: Go/C++/Java
> Reporter: Adam Beberg
> Original Estimate: 0.5h
> Remaining Estimate: 0.5h
>
> The code introduced in THRIFT-3027 (now closed) created issues.
> 1. It's inconsistent. Compare thrift name and Go names:
> {code}
> type Foo struct {
> Id int32 `thrift:"id,1" json:"id"`
> MyID int32 `thrift:"my_id,2" json:"my_id"`
> NumCPU int32 `thrift:"num_cpu,3" json:"num_cpu"`
> NumGpu int32 `thrift:"num_gpu,4" json:"num_gpu"`
> My_ID int32 `thrift:"my_ID,5" json:"my_ID"`
> }
> {code}
> CPU vs Gpu, Id vs ID, ...
> 2. It's one-way. This is a serious problem. Go code dealing with databases
> and other things assumes convertibility between snake_case and CamelCase in
> both directions for things like SQL column names. This breaks that.
> 3. Generated code is explicitly NOT subject to this in the Go initialism
> guideline, for good reasons.
> "Code generated by the protocol buffer compiler is exempt from this rule.
> Human-written code is held to a higher standard than machine-written code."
> 4. Working across Go/C++/Java, these inconsistencies are really messing with
> engineers that aren't as familiar with Thrift internals, which will
> inevitably lead to mistakes, so it's a usability issue.
> My recommendation: Strongly suggest removing the initialism rewriting code,
> those wanting initials in caps can still specify them in the thrift
> definitions without any problems in Go (but then Java has problems). If not
> make it a command line option usable by those working only in Go without
> other dependencies, but that still leaves problems 1 & 3.
> I can quickly prepare a patch to remove or flag-ify, but would like
> discussion.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
