Hyphenated file support #424

abhishekparwal · 2019-10-10T00:26:46Z

added support for the Hyphenated files as discussed in #143

Details:
Go Code generated would be having a package name as abc_def, i.e hyphen converted to underscore for working around golang casing constraints.

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. however, #423 adds support for it, which we are not landing now.

peats-bond

Thanks for the PR!

I primarily have issues with style and readability. Some of the functionality will be easier to review after some clean up.

peats-bond · 2019-10-11T00:32:36Z

gen/internal/tests/Makefile

@@ -18,4 +18,4 @@ nozap: thrift/nozap.thrift $(THRIFTRW)
 	$(THRIFTRW) --no-recurse --no-zap $<

 %: thrift/%.thrift $(THRIFTRW)
-	$(THRIFTRW) --no-recurse $<
+	$(THRIFTRW) $<


Why did we drop this?

we have nested directory and it should recursively try to generate code.

gen/generate.go

peats-bond · 2019-10-11T00:48:19Z

gen/generate.go

+// fileExists checks if a file exists and is not a directory before we
+// try using it to prevent further errors.


the function is already self-documenting and it seems odd indicating the errors here.
Consider simplifying to fileExists checks if a file exists or drop the comment altogether

sure dropping function comments

gen/generate.go

gen/internal/tests/thrift/abc-defs.thrift

gen/generate.go

peats-bond · 2019-10-11T01:26:58Z

gen/generate.go

@@ -105,6 +105,10 @@ func Generate(m *compile.Module, o *Options) error {
 	genBuilder := newGenerateServiceBuilder(importer)

 	generate := func(m *compile.Module) error {
+		if err := isDuplicateFileAfterNormalization(m.ThriftPath); err != nil {


This name is a bit of a mouthful, consider renaming to something more succinct.

gen/generate_test.go

compile/error.go

peats-bond · 2019-10-11T21:15:37Z

compile/compiler.go

+// Among other errors, one place it will error out if you attempt to include
+// a file whose name starts with hyphen.


It's not "starts" right, it's "contains"? We can make this short and sweet:

Including hyphenated file names will error.

okay changing the message.

peats-bond · 2019-10-11T21:17:28Z

compile/compiler.go

@@ -251,6 +252,14 @@ func (c compiler) include(m *Module, include *ast.Include) (*IncludedModule, err
 		}
 	}

+	includeName := fileBaseName(include.Path)
+	if strings.Contains(includeName, "-") {


Thoughts on collapsing this still? The intermediate variable doesn't save us much here.

compile/error.go

gen/generate.go

gen/generate_test.go

peats-bond · 2019-10-11T21:30:27Z

gen/generate_test.go

+			filepath: "internal/tests/thrift/hyphenated-file.thrift",
+		},
+		{
+			name:          "hyphen file post normalization code gen fails with non hyphen same name file",


Suggested change

name: "hyphen file post normalization code gen fails with non hyphen same name file",

name: "normalization collision err",

making the change, though I consider previous messages as more descriptive and easy to understand.

gen/generate_test.go

peats-bond

This looks a lot cleaner thanks for the clean up! 👍
Functionally this looks fine to me, I'll lean on @mh-park / @rhang-uber / @abhinav to please follow up with their review when they get a chance.

gen/generate.go

peats-bond · 2019-10-11T21:41:55Z

gen/generate.go

@@ -186,7 +222,7 @@ type thriftPackageImporter struct {
 }

 func (i thriftPackageImporter) RelativePackage(file string) (string, error) {
-	return filepath.Rel(i.ThriftRoot, strings.TrimSuffix(file, ".thrift"))
+	return filepath.Rel(i.ThriftRoot, strings.TrimSuffix(normalizeFilePath(file), ".thrift"))


Since we pass in the normalized path on line 272, can we remove this change?

RelativePackage is responsible for generating the import package name, as below, hence needed, while the line 272 is there for generating the base package name of file for code gen.

// ThriftModule represents the IDL file used to generate this package. var ThriftModule = &thriftreflect.ThriftModule{ Name: "hyphenated-file", Package: "go.uber.org/thriftrw/gen/internal/tests/hyphenated_file", FilePath: "hyphenated-file.thrift", SHA1: "8513913ac76a3ba1c6b2b3b6fb241462e162c446", Includes: []*thriftreflect.ThriftModule{ non_hyphenated.ThriftModule, }, Raw: rawIDL, }

RohanRathi · 2019-10-14T06:26:11Z

Hi, when can this PR be merged? My work is pending on its approval

abhinav · 2019-10-14T15:53:17Z

Apologies for the delay @abhishekparwal, @RohanRathi. We've been pretty under-
water with other higher priority work, so there hasn't been ample opportunity
to thoroughly review this change. We'll try to give it more review passes as
soon as we're able.

This is a nice graceful degradation for certain failure scenarios, but it
shouldn't be a blocker for any work. It's a matter of renaming your file. It
should be perfectly safe to rename those files because no other Thrift file
can legally include and use them.

abhinav

Sorry, this approach is not a safe approach for this change. It changes the
compiler to read an inaccurate, lossy representation of on-disk state. We have
tried to maintain the invariant that the compiler always reads an accurate
representation of on-disk state with no language-specific concerns or
transformations. Put differently, it should theoretically be possible to
recreate a near-copy of the original Thrift files, which includes their file
names, from the compiled representation of those Thrift files.

Go-specific concerns and relevant transformations are scoped to the gen/
package in ThriftRW. That the name the Thrift file affects the name of the
generated Go package is a concern limited to that directory. This means that
this feature needs no changes outside that directory (barring the hyphenated-
include check which, theoretically, could be its own change).

The lossy conversion actually loses some functionality: it makes it impossible
for foo-bar.thrift and foo_bar.thrift to coexist in the same directory
even if they and all their consumers are completely disjoint.

For this feature, I think the following should hold:

If a file name is foo-bar.thrift, everything remains the same. The code
still gets generated inside a directory named foo-bar/. Only the package
name in the generated code is different: foo_bar.
Another file named foo_bar.thrift may coexist in the same directory as
foo-bar.thrift. Its code goes into the directory foo_bar/, with the
package name foo_bar.

gen/generate.go

abhishekparwal

Sorry, this approach is not a safe approach for this change. It changes the
compiler to read an inaccurate, lossy representation of on-disk state. We have
tried to maintain the invariant that the compiler always reads an accurate
representation of on-disk state with no language-specific concerns or
transformations. Put differently, it should theoretically be possible to
recreate a near-copy of the original Thrift files, which includes their file
names, from the compiled representation of those Thrift files.

Go-specific concerns and relevant transformations are scoped to the gen/
package in ThriftRW. That the name the Thrift file affects the name of the
generated Go package is a concern limited to that directory. This means that
this feature needs no changes outside that directory (barring the hyphenated-
include check which, theoretically, could be its own change).

The lossy conversion actually loses some functionality: it makes it impossible
for foo-bar.thrift and foo_bar.thrift to coexist in the same directory
even if they and all their consumers are completely disjoint.

For this feature, I think the following should hold:

If a file name is foo-bar.thrift, everything remains the same. The code
still gets generated inside a directory named foo-bar/. Only the package
name in the generated code is different: foo_bar.

Another file named foo_bar.thrift may coexist in the same directory as
foo-bar.thrift. Its code goes into the directory foo_bar/, with the
package name foo_bar.

I made changes only in gen. Other than that only change is hyphenated include check.
foo-bar.thrift and foo_bar.thrift can coexist. only if you try to use foo-bar.thrift, it will error out. already have test cases to prove that.
having a foo_bar or foo-bar directory is a code generation nuance. I don't think generation nuance of foo-bar or foo_bar should be enforced. Also, currently folder name and package name comes from the same method: RelativePackage and thus are indistinguishable from code-gen perspective. However, if you are strongly suggesting this, I can see the changes needed.

abhishekparwal · 2019-10-11T21:50:28Z

compile/compiler.go

+// Among other errors, one place it will error out if you attempt to include
+// a file whose name starts with hyphen.


okay changing the message.

abhishekparwal · 2019-10-11T21:52:40Z

compile/compiler.go

@@ -251,6 +252,14 @@ func (c compiler) include(m *Module, include *ast.Include) (*IncludedModule, err
 		}
 	}

+	includeName := fileBaseName(include.Path)
+	if strings.Contains(includeName, "-") {


compile/error.go

gen/generate.go

abhishekparwal · 2019-10-11T21:58:30Z

gen/generate_test.go

+			filepath: "internal/tests/thrift/hyphenated-file.thrift",
+		},
+		{
+			name:          "hyphen file post normalization code gen fails with non hyphen same name file",


making the change, though I consider previous messages as more descriptive and easy to understand.

gen/generate_test.go

gen/generate.go

abhishekparwal · 2019-10-11T22:11:56Z

gen/generate.go

@@ -186,7 +222,7 @@ type thriftPackageImporter struct {
 }

 func (i thriftPackageImporter) RelativePackage(file string) (string, error) {
-	return filepath.Rel(i.ThriftRoot, strings.TrimSuffix(file, ".thrift"))
+	return filepath.Rel(i.ThriftRoot, strings.TrimSuffix(normalizeFilePath(file), ".thrift"))


RelativePackage is responsible for generating the import package name, as below, hence needed, while the line 272 is there for generating the base package name of file for code gen.

// ThriftModule represents the IDL file used to generate this package. var ThriftModule = &thriftreflect.ThriftModule{ Name: "hyphenated-file", Package: "go.uber.org/thriftrw/gen/internal/tests/hyphenated_file", FilePath: "hyphenated-file.thrift", SHA1: "8513913ac76a3ba1c6b2b3b6fb241462e162c446", Includes: []*thriftreflect.ThriftModule{ non_hyphenated.ThriftModule, }, Raw: rawIDL, }

abhinav · 2019-10-14T19:07:30Z

Hey,

So you have. I must've misread. My apologies.
I'll need to take a second pass to verify but I thought I read something that would break this.
As discussed offline, the expectation is that path/to/thrift/file.thrift becomes path/to/thrift/file/*.go, whatever file is. We just need to change the generated package name, package $(basename $file) to package $(normalize $(basename $file)).

… path/to/file.thrift becomes path/to/file/*.go. Previously, file.thrift became package $file. Now it would be package $(normalize $file)

abhishekparwal · 2019-10-14T21:31:08Z

Hey,

So you have. I must've misread. My apologies.

I'll need to take a second pass to verify but I thought I read something that would break this.

As discussed offline, the expectation is that path/to/thrift/file.thrift becomes path/to/thrift/file/*.go, whatever file is. We just need to change the generated package name, package $(basename $file) to package $(normalize $(basename $file)).

TestGenerateWithHyphenPaths/normalization are the name of test/subtests
have modified to generate the package and not change the name of the generated go file.

gen/generate.go

abhinav

Looks good; minor fixes needed.

Thanks for dealing with all the back and forth.

gen/generate_test.go

gen/internal/tests/thrift/hyphenated_file.thrift

Co-Authored-By: Abhinav Gupta <mail@abhinavg.net>

…w-go#424

* glide up to pull in hyphenated file fix in trhuftrw: thriftrw/thriftrw-go#424 * generate code

Hyphenated file support

1c47496

abhishekparwal mentioned this pull request Oct 10, 2019

Dashes in Thrift file names #143

Open

gen code

6c0ab1d

abhinav requested a review from r-hang October 11, 2019 00:04

peats-bond reviewed Oct 11, 2019

View reviewed changes

abhishekparwal added 3 commits October 10, 2019 21:15

review comments

61f08cd

generated go code

232c51a

test msg assert

68ba6db

peats-bond reviewed Oct 11, 2019

View reviewed changes

stylistic code comments

bc12fe2

abhinav suggested changes Oct 14, 2019

View reviewed changes

gen/generate.go Outdated Show resolved Hide resolved

gen/generate.go Outdated Show resolved Hide resolved

gen/generate.go Outdated Show resolved Hide resolved

abhishekparwal commented Oct 14, 2019

View reviewed changes

abhishekparwal added 4 commits October 14, 2019 12:38

code review comments

ef2c62f

addressing paths to be retained and just package to be renamed, i.e.…

e508f24

… path/to/file.thrift becomes path/to/file/*.go. Previously, file.thrift became package $file. Now it would be package $(normalize $file)

fix test

7add654

undoing accidental comment

ac2027f

quick test - loading the generated types

53bdd81

abhinav reviewed Oct 15, 2019

View reviewed changes

gen/generate.go Outdated Show resolved Hide resolved

review comments

c8cbaa1

abhinav approved these changes Oct 16, 2019

View reviewed changes

gen/generate_test.go Outdated Show resolved Hide resolved

gen/generate_test.go Outdated Show resolved Hide resolved

gen/internal/tests/thrift/hyphenated_file.thrift Show resolved Hide resolved

abhishekparwal and others added 4 commits October 16, 2019 13:34

Update gen/generate_test.go

9f30c30

Co-Authored-By: Abhinav Gupta <mail@abhinavg.net>

Update gen/internal/tests/thrift/hyphenated_file.thrift

a6e531d

Co-Authored-By: Abhinav Gupta <mail@abhinavg.net>

Update gen/generate_test.go

2187e99

Co-Authored-By: Abhinav Gupta <mail@abhinavg.net>

generated file

cf2b8e2

abhinav merged commit 0d8d9fe into thriftrw:dev Oct 16, 2019

abhishekparwal added a commit to uber/zanzibar that referenced this pull request Oct 18, 2019

glide up to pull in hyphenated file fix in trhuftrw: thriftrw/thriftr…

660ec77

…w-go#424

abhishekparwal mentioned this pull request Oct 18, 2019

Thriftrw bump - hyphenated file fix in thriftrw-go uber/zanzibar#662

Merged

abhishekparwal added a commit to uber/zanzibar that referenced this pull request Oct 18, 2019

Thriftrw bump - hyphenated file fix in thriftrw-go (#662)

6d3b183

* glide up to pull in hyphenated file fix in trhuftrw: thriftrw/thriftrw-go#424 * generate code

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hyphenated file support #424

Hyphenated file support #424

abhishekparwal commented Oct 10, 2019 •

edited

peats-bond left a comment

peats-bond Oct 11, 2019

abhishekparwal Oct 11, 2019

peats-bond Oct 11, 2019

abhishekparwal Oct 11, 2019

peats-bond Oct 11, 2019

abhishekparwal Oct 11, 2019

peats-bond Oct 11, 2019

abhishekparwal Oct 11, 2019

peats-bond Oct 11, 2019

peats-bond Oct 11, 2019

abhishekparwal Oct 11, 2019

peats-bond left a comment •

edited

peats-bond Oct 11, 2019

abhishekparwal Oct 11, 2019

RohanRathi commented Oct 14, 2019

abhinav commented Oct 14, 2019

abhinav left a comment

abhishekparwal left a comment

abhishekparwal Oct 11, 2019

abhishekparwal Oct 11, 2019

abhishekparwal Oct 11, 2019

abhishekparwal Oct 11, 2019

abhinav commented Oct 14, 2019

abhishekparwal commented Oct 14, 2019

abhinav left a comment

		// fileExists checks if a file exists and is not a directory before we
		// try using it to prevent further errors.

		// Among other errors, one place it will error out if you attempt to include
		// a file whose name starts with hyphen.

	name: "hyphen file post normalization code gen fails with non hyphen same name file",
	name: "normalization collision err",

Hyphenated file support #424

Hyphenated file support #424

Conversation

abhishekparwal commented Oct 10, 2019 • edited

peats-bond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peats-bond left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RohanRathi commented Oct 14, 2019

abhinav commented Oct 14, 2019

abhinav left a comment

Choose a reason for hiding this comment

abhishekparwal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinav commented Oct 14, 2019

abhishekparwal commented Oct 14, 2019

abhinav left a comment

Choose a reason for hiding this comment

abhishekparwal commented Oct 10, 2019 •

edited

peats-bond left a comment •

edited