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

Hyphenated file support #424

Merged
merged 16 commits into from
Oct 16, 2019

Conversation

abhishekparwal
Copy link
Contributor

@abhishekparwal abhishekparwal commented Oct 10, 2019

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.

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

@peats-bond peats-bond left a comment

Choose a reason for hiding this comment

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

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.

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

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

Choose a reason for hiding this comment

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

Why did we drop this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

gen/generate.go Outdated Show resolved Hide resolved
gen/generate.go Outdated Show resolved Hide resolved
gen/generate.go Outdated
Comment on lines 191 to 192
// fileExists checks if a file exists and is not a directory before we
// try using it to prevent further errors.

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure dropping function comments

gen/generate.go Outdated Show resolved Hide resolved
gen/internal/tests/thrift/abc-defs.thrift Outdated Show resolved Hide resolved
gen/generate.go Outdated Show resolved Hide resolved
gen/generate.go Outdated
@@ -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 {

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

gen/generate_test.go Show resolved Hide resolved
compile/error.go Outdated Show resolved Hide resolved
Comment on lines 246 to 247
// Among other errors, one place it will error out if you attempt to include
// a file whose name starts with hyphen.

Choose a reason for hiding this comment

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

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

Including hyphenated file names will error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay changing the message.

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

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

Choose a reason for hiding this comment

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

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

compile/error.go Outdated Show resolved Hide resolved
gen/generate.go Outdated Show resolved Hide resolved
gen/generate.go Outdated Show resolved Hide resolved
gen/generate_test.go Outdated Show resolved Hide resolved
filepath: "internal/tests/thrift/hyphenated-file.thrift",
},
{
name: "hyphen file post normalization code gen fails with non hyphen same name file",

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

gen/generate_test.go Outdated Show resolved Hide resolved
gen/generate_test.go Outdated Show resolved Hide resolved
gen/generate_test.go Outdated Show resolved Hide resolved
Copy link

@peats-bond peats-bond 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 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 Outdated Show resolved Hide resolved
gen/generate.go Outdated
@@ -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"))

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link

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

@abhinav
Copy link
Contributor

abhinav commented Oct 14, 2019

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.

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.

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 Outdated Show resolved Hide resolved
gen/generate.go Outdated Show resolved Hide resolved
gen/generate.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@abhishekparwal abhishekparwal left a comment

Choose a reason for hiding this comment

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

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.
  1. I made changes only in gen. Other than that only change is hyphenated include check.

  2. 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.

  3. 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.

Comment on lines 246 to 247
// Among other errors, one place it will error out if you attempt to include
// a file whose name starts with hyphen.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay changing the message.

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

includeName := fileBaseName(include.Path)
if strings.Contains(includeName, "-") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

compile/error.go Outdated Show resolved Hide resolved
gen/generate.go Outdated Show resolved Hide resolved
gen/generate.go Outdated Show resolved Hide resolved
filepath: "internal/tests/thrift/hyphenated-file.thrift",
},
{
name: "hyphen file post normalization code gen fails with non hyphen same name file",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

gen/generate_test.go Outdated Show resolved Hide resolved
gen/generate_test.go Outdated Show resolved Hide resolved
gen/generate.go Outdated Show resolved Hide resolved
gen/generate.go Outdated
@@ -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"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

abhinav commented Oct 14, 2019

Hey,

  1. So you have. I must've misread. My apologies.
  2. I'll need to take a second pass to verify but I thought I read something that would break this.
  3. 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
Copy link
Contributor Author

Hey,

  1. So you have. I must've misread. My apologies.
  2. I'll need to take a second pass to verify but I thought I read something that would break this.
  3. 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)).
  1. TestGenerateWithHyphenPaths/normalization are the name of test/subtests

  2. have modified to generate the package and not change the name of the generated go file.

gen/generate.go Outdated Show resolved Hide resolved
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.

Looks good; minor fixes needed.

Thanks for dealing with all the back and forth.

gen/generate_test.go Outdated Show resolved Hide resolved
gen/generate_test.go Outdated Show resolved Hide resolved
@abhinav 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/thriftrw-go#424

* generate code
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.

4 participants