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

gen/enum: Support periods in item names #407

Closed
wants to merge 6 commits into from

Conversation

ramprasadg
Copy link

@ramprasadg ramprasadg commented Feb 22, 2019

This adds support for enum items with periods in their names.

Previously, the following enum would parse successfully but generate
invalid Go code.

enum X { foo.bar, baz }

With this change, foo.bar is treated similarly to foo_bar.

@CLAassistant
Copy link

CLAassistant commented Feb 22, 2019

CLA assistant check
All committers have signed the CLA.

@abhinav
Copy link
Contributor

abhinav commented Feb 22, 2019

Hello! Thanks for your contribution! Before we can review this, please,

  • Sign the CLA.
  • Provide a descriptive PR title.
  • Provide a detailed PR description that explains what this change is doing and why; the how should be self-evident from the code.
  • If you're adding new behavior, add or update a test to exercise this behavior. For code generation changes, check out gen/internal/tests/thrift.

@ramprasadg ramprasadg changed the title split names on fullstop also Support enumItems with fullstop/period. Split names on fullstop also Feb 26, 2019
@ramprasadgopal
Copy link

@abhinav can you please have a look now?
I have added the unit tests.

Copy link
Contributor

@abhinav abhinav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Thank you!

CC @kriskowal @mh-park if you wouldn't mind taking a look as well.

@abhinav abhinav changed the title Support enumItems with fullstop/period. Split names on fullstop also gen/enum: Support periods in item names Feb 27, 2019
@abhinav abhinav self-requested a review February 27, 2019 17:28
Copy link
Contributor

@abhinav abhinav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I just realized that this isn't something that Apache Thrift
supports. Introducing this change will encourage Thrift files that work with
ThriftRW but not with Apache Thrift.

$ cat > tmp.thrift
enum X { foo.bar, baz }
$ thrift --gen py tmp.thrift
[ERROR:/Users/abg/tmp/tmp.thrift:1] (last token was ',')
Identifier foo.bar can't have a dot.
$ thrift --gen go tmp.thrift
[ERROR:/Users/abg/tmp/tmp.thrift:1] (last token was ',')
Identifier foo.bar can't have a dot.

Can you add some background about your use case so that we can weigh the
potential issues associated with this change and possible solutions?

@ramprasadg
Copy link
Author

@abhinav its because RTAPI thrift files use them.
looks is Android, iOS code gens unfortunately support this non standard version.

@kriskowal
Copy link
Contributor

@ramprasadg We’ve had many conversations over the years about RTAPI’s enums. The road for RTAPI IDL to be welcomed into the fold of IDL that can cross-compile is to move the dot delimited name into an annotation and change the name to an identifier that will compile in other languages. This is important especially since Eats services in particular use RTAPI to translate JSON to TBinary for Go services, so the IDL needs to compile for both.

I was under the impression that there was an effort in progress to convert enum names to annotations.

@ramprasadg
Copy link
Author

@kriskowal do you know who would be the right people to contact?
do you see a problem with this commit as an interim solution until the RTAPI effort is completed?

@abhinav
Copy link
Contributor

abhinav commented May 19, 2021

We decided against this after all.

@abhinav abhinav closed this May 19, 2021
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.

None yet

5 participants