Camel casing that is better backward compatible with scrooge 3.0.3. #53

Merged
merged 1 commit into from Mar 18, 2013

Conversation

Projects
None yet
3 participants
Contributor

erikvanoosten commented Mar 15, 2013

This patch changes the way camel casing is created. The goal of the change is
to be more compatible with Scrooge 3.0.3. In particular words like HTML_REPORT
are now again converted to HtmlReport in the Scala output.

Note that the change is also quite compatible with other spellings as none of
the tests (except for one detailed unit test) needed to be changed.

Contributor

erikvanoosten commented Mar 15, 2013

This pull request solved issue #45.

Member

kevinoliver commented Mar 15, 2013

Pretty sure @chunyan made this change on purpose…

Contributor

erikvanoosten commented Mar 16, 2013

I know. But she didn't think of the case where the thrift file contains enum values in all upper case. Look at her commit message ( ffa634f ):

Use correct case for enum fields on the RHS

Add genEnum methods to both Java and Scala generators. For enum fields, Java uses upper case, Scala uses title case. We didn't differentiate this before when it's on the RHS

Her goal was to use title case for Scala, however with input like HTML_REPORTER you will get HTMLREPORTER as object name (obviously not title case), where as before it was HtmlReporter.

This pull request preserves all @chunyan 's changes except for the case where the thrift input is in all upper case.

Contributor

chunyan commented Mar 16, 2013

@erikvanoosten, aahh I just realized that there was a change that I forgot to document in 3.0.7, that Scrooge now preserves consecutive upper case letters in title case. So HTML_REPORTER will be HTMLREPORTER not HtmlReporter. Sorry for the confusion.

Contributor

chunyan commented Mar 16, 2013

@erikvanoosten thanks for the pull request, and I think what you propose makes a lot of sense. But without a uniform standard for title case, I don't think it's possible to bring everyone on the same page on this issue.
I apologize for the breaking changes. And unfortunately there will be more coming as a result of addressing the current priorities at Twitter. I hope you understand and please continue to support this project.

chunyan closed this Mar 16, 2013

Contributor

erikvanoosten commented Mar 17, 2013

@chunyan I understand you want to preserve consecutive upper case letters. Something like HTMLReporter should definitely stay HTMLReporter. This pull request keeps that behavior.

We are heavy users of the Apache compiler also. As we are moving more and more to Scala code we now have 1 service using Scrooge. Anyways, as we use the Apache compiler so much, we have enum values all written in all upper case with underscores to separate the words. Just as you would do in a Java enum.

To stress the point: we have thousands of lines of code here at ebay that depend on this. We really can not afford a change like this.

Perhaps we can find a middle ground. What if we preserve case unless the entire word is written in upper case?

Contributor

chunyan commented Mar 18, 2013

Hi @erikvanoosten
Yeah, let's try to find a middle ground. Your broken case is that you have something like HTML_RESPONSE, and you've already took dependency on the fact that it used to map to HtmlResponse, is that right? Anything else that's broken?

HTML_RESPONSE

chunyan reopened this Mar 18, 2013

Contributor

chunyan commented Mar 18, 2013

oops, sent before I finish. I will comment on individual test cases in your code

@chunyan chunyan commented on the diff Mar 18, 2013

...c/main/scala/com/twitter/scrooge/AST/Identifier.scala
*
* Examples: (original, camel case, title case)
- * (genID, genID, GenID)
- * (_genID, _genID, _GenID)
- * (gen_id, genId, GenId)
+ * (gen_html_report, genHtmlReport, GenHtmlReport)
+ * (GEN_HTML_REPORT, genHtmlReport, GenHtmlReport)
+ * (Gen_HTMLReport, genHTMLReport, GenHTMLReport)
+ * (Gen_HTML_Report, genHtmlReport, GenHtmlReport)
+ * (GENHTMLREPORT, genhtmlreport, Genhtmlreport)
+ * (genhtmlreport, genhtmlreport, Genhtmlreport)
+ * (genHtmlReport, genHtmlReport, GenHtmlReport)
+ * (_genHtmlReport, _genHtmlReport, _GenHtmlReport)
*/
@chunyan

chunyan Mar 18, 2013

Contributor

Add a case:
(genHTMLReport, genHTMLReport, GenHTMLReport)

Contributor

chunyan commented Mar 18, 2013

Actually let's go with your current plan. Just add a test case (genHTMLReport, genHTMLReport, GenHTMLReport).

We treat all-letter whole strings the same way as each segment of a string separated by underscores. If it's all cap, changes to camel and title case; if not all cap, preserver internal caps.

chunyan merged commit a6f3d47 into twitter:master Mar 18, 2013

1 check passed

default The Travis build passed
Details
Contributor

erikvanoosten commented Mar 19, 2013

Thanks @chunyan , that is much appreciated.

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