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

Because, because... fun, the scala compiler has special naming rules … #1534

Merged
merged 1 commit into from
Mar 4, 2016

Conversation

ianoc
Copy link
Collaborator

@ianoc ianoc commented Mar 4, 2016

…it appears when there are leading underscores

fixes #1528

/cc @johnynek

…it appears when there are leading underscores
@johnynek
Copy link
Collaborator

johnynek commented Mar 4, 2016

Nice catch. Why the toLowerCase?

@ianoc
Copy link
Collaborator Author

ianoc commented Mar 4, 2016

We do this to try work out the methods which are the field names for the trait, since they aren't case class accessor, we sort of have a guessing game.

The reason for the toLowerCase is because the compiler does capitalization of the first letter of the field name, except if the leading characters are underscores, then its the first non underscore character it seems.

So to just try sidestep the whole mess I went with the toLowerCase, the risk there becomes i guess of a false positive, but I couldn't manage to figure out how to generate something where it doesn't work that is valid thrift.

@johnynek
Copy link
Collaborator

johnynek commented Mar 4, 2016

Can we add an example with a collision in the upper and lower casing? a and A as fields? Is that allowed?

@johnynek
Copy link
Collaborator

johnynek commented Mar 4, 2016

Could we just call the Scrooge method that is doing this mangling? We already depend on Scrooge, right?

@ianoc
Copy link
Collaborator Author

ianoc commented Mar 4, 2016

well sorry i mean i you can do it if you put long enough field names to avoid the auto-casing. But if you add them as fields, then they are actually fields... so they will be there anyway. They are just there twice.

@ianoc
Copy link
Collaborator Author

ianoc commented Mar 4, 2016

Its different mangling, this is the scala compiler mangling, not thrift -> scrooge

@johnynek
Copy link
Collaborator

johnynek commented Mar 4, 2016

Ahh. I see.

Well, this is an improvement for sure. Merge when green.

On Thursday, March 3, 2016, ianoc notifications@github.com wrote:

Its different mangling, this is the scala compiler mangling, not thrift ->
scrooge


Reply to this email directly or view it on GitHub
#1534 (comment).

P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin

ianoc added a commit that referenced this pull request Mar 4, 2016
Because, because... fun, the scala compiler has special naming rules …
@ianoc ianoc merged commit e7395b3 into develop Mar 4, 2016
@ianoc ianoc deleted the ianoc/fixScroogeMacroHandling branch March 4, 2016 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug in thrift ordered serialization macros
3 participants