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

Dashes in Thrift file names #143

Open
abhinav opened this issue May 27, 2016 · 5 comments
Open

Dashes in Thrift file names #143

abhinav opened this issue May 27, 2016 · 5 comments

Comments

@abhinav
Copy link
Contributor

abhinav commented May 27, 2016

We need to replace dashes in file names with underscores.

$ cat > hello-world.thrift
struct Hello { 1: required string world }
$ thriftrw-go hello-world.thrift
$ head hello-world/types.go
// Code generated by thriftrw

package hello-world
[..]

Also, if we add a service, service Foo { Hello bar() },

$ thriftrw-go hello-world.thrift
2016/05/27 15:57:28 failed to generate code for "[..]/hello-world.thrift": could not generate code for service "Foo": could not generate types for Foo.bar: failed to generate code for "Foo.bar": could not parse generated code: thriftrw.go:6:20: expected ';', found '-' (and 1 more errors):
package thriftrw

type BarResult struct {


                    Success *hello-world.Hello `json:"success,omitempty"`


        }
@abhinav
Copy link
Contributor Author

abhinav commented May 27, 2016

A similar issue applies to Thrift files named after Go keywords: for, return, switch, type, etc.

@abhinav abhinav added this to the 1.0.0 milestone Sep 22, 2016
@abhinav abhinav modified the milestone: 1.0.0 Nov 2, 2016
@abhinav
Copy link
Contributor Author

abhinav commented Oct 8, 2019

Follow up:
Apache Thrift does not support this either. In fact, files with dashes in
their names cannot be included by other Thrift files.

include "foo-bar.thrift"

The Thrift IDL format does not provide a means of referencing identifiers
defined in foo-bar.thrift. It does not get renamed to foo_bar.

It still makes sense for us to have a graceful degradation when generating
code for top-level Thrift files that aren't included by other files, so it
should be okay to make the code-generator more accepting without changing the
parsing behavior.

@abhishekparwal
Copy link
Contributor

direct include is a limitation of thrift grammar which doesn't allow hyphenated names to be used as an identifier, like abc-def.XYZ xyzis disallowed. however we can do include-as to even include these files, which renders the use of these files beyond top level.

#423 adds support for the Hyphenated file as both top level thrift files as well as included thrift file.

include-as is similar to golang import alias, i.e.:

include t "abc-def.thrift"
t.XYZ

Hyphenated files normally can not be included as identifier of that form are not allowed in thrift grammar. This is because of hyphens like abc-def.XYZ. However, if the include-as directive is used, even with hyphen names we can use like include t "abc-def.thrift" t.XYZ. Since include-as was not supported previously added support for it as now even these thrift files are usable other than top level files.

Follow up:
Apache Thrift does not support this either. In fact, files with dashes in
their names cannot be included by other Thrift files.

include "foo-bar.thrift"

The Thrift IDL format does not provide a means of referencing identifiers
defined in foo-bar.thrift. It does not get renamed to foo_bar.

It still makes sense for us to have a graceful degradation when generating
code for top-level Thrift files that aren't included by other files, so it
should be okay to make the code-generator more accepting without changing the
parsing behavior.

@abhinav
Copy link
Contributor Author

abhinav commented Oct 9, 2019

include t "abc-def.thrift"

That syntax is not recognized by Apache Thrift. We cannot add new syntax to
the Thrift IDL format. Files that use custom syntax will fail to be processed
by Apache Thrift, which will necessitate custom implementations of Thrift in
languages that we do not currently have custom implementations in. This is
something we would like to avoid.

@abhishekparwal
Copy link
Contributor

include t "abc-def.thrift"

That syntax is not recognized by Apache Thrift. We cannot add new syntax to
the Thrift IDL format. Files that use custom syntax will fail to be processed
by Apache Thrift, which will necessitate custom implementations of Thrift in
languages that we do not currently have custom implementations in. This is
something we would like to avoid.

#424 adds support for the Hyphenated files. Go Code generated would be having a package name as abc_def, i.e hyphen converted to underscore for working around golang casing constraints.

however, as @abhinav said, Hyphenated files can not be included as an identifier of that form are not allowed in thrift grammar. This is because of usages with hyphens like abc-def.XYZ.
Not adding support for include-as directive as this is a custom thrift syntax which is not officially supported and leaked into thriftrw grammar earlier. though, #423 tried adding support for it, which we are not landing now.

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

No branches or pull requests

2 participants