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
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions compile/compiler.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ package compile

import (
"path/filepath"
"strings"

"go.uber.org/thriftrw/ast"
"go.uber.org/thriftrw/idl"
Expand Down Expand Up @@ -242,6 +243,7 @@ func (c compiler) gather(m *Module, prog *ast.Program) error {
// include loads the file specified by the given include in the given Module.
//
// The path to the file is relative to the ThriftPath of the given module.
// Including hyphenated file names will error.
func (c compiler) include(m *Module, include *ast.Include) (*IncludedModule, error) {
if len(include.Name) > 0 {
// TODO(abg): Add support for include-as flag somewhere.
Expand All @@ -251,6 +253,13 @@ func (c compiler) include(m *Module, include *ast.Include) (*IncludedModule, err
}
}

if strings.Contains(fileBaseName(include.Path), "-") {
return nil, includeError{
Include: include,
Reason: includeHyphenatedFileNameError{},
}
}

ipath := filepath.Join(filepath.Dir(m.ThriftPath), include.Path)
incM, err := c.load(ipath)
if err != nil {
Expand Down
8 changes: 8 additions & 0 deletions compile/error.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,14 @@ func (e includeAsDisabledError) Error() string {
return "include-as syntax is currently disabled"
}

// includeHyphenatedFileNameError is raised when the user attempts to
// include hyphenated file names.
type includeHyphenatedFileNameError struct{}

func (e includeHyphenatedFileNameError) Error() string {
return "cannot include hyphenated Thrift files"
}

// includeError is raised when there is an error including another Thrift
// file.
type includeError struct {
Expand Down
14 changes: 10 additions & 4 deletions gen/generate.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,11 @@ func Generate(m *compile.Module, o *Options) error {
return nil
}

// normalizePackageName replaces hyphens in the file name with underscores.
func normalizePackageName(p string) string {
return strings.Replace(filepath.Base(p), "-", "_", -1)
}

// ThriftPackageImporter determines import paths from a Thrift root.
type ThriftPackageImporter interface {
// RelativePackage returns the import path for the top-level package of the
Expand Down Expand Up @@ -235,12 +240,11 @@ func generateModule(
if err != nil {
return "", nil, err
}

// TODO(abg): Prefer top-level package name from `namespace go` directive.
packageName := filepath.Base(packageRelPath)
outputFilename := filepath.Base(packageRelPath)

// Output file name defaults to the package name.
outputFilename := packageName + ".go"
outputFilename = outputFilename + ".go"
if len(o.OutputFile) > 0 {
outputFilename = o.OutputFile
}
Expand All @@ -253,10 +257,12 @@ func generateModule(
return "", nil, err
}

// converts package name from ab-def to ab_def for golang code generation
normalizedPackageName := normalizePackageName(filepath.Base(packageRelPath))
g := NewGenerator(&GeneratorOptions{
Importer: i,
ImportPath: importPath,
PackageName: packageName,
PackageName: normalizedPackageName,
NoZap: o.NoZap,
})

Expand Down
56 changes: 56 additions & 0 deletions gen/generate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,62 @@ func TestGenerateWithRelativePaths(t *testing.T) {
}
}

func TestGenerateWithHyphenPaths(t *testing.T) {
outputDir, err := ioutil.TempDir("", "thriftrw-generate-test")
peats-bond marked this conversation as resolved.
Show resolved Hide resolved
require.NoError(t, err)
defer os.RemoveAll(outputDir)

thriftRoot, err := os.Getwd()
require.NoError(t, err)

tests := []struct {
name string
filepath string
peats-bond marked this conversation as resolved.
Show resolved Hide resolved
generateError string
compileError string
}{
{
name: "include with hyphens error",
filepath: "internal/tests/thrift/nestedfiles_error/include_hyphen_files.thrift",
compileError: "cannot include hyphenated Thrift files",
},
{
name: "normalization - hyphen files as top level file allowed",
abhishekparwal marked this conversation as resolved.
Show resolved Hide resolved
filepath: "internal/tests/thrift/hyphenated-file.thrift",
},
{
name: "normalization - non hyphen file with hyphen in the directory",
abhishekparwal marked this conversation as resolved.
Show resolved Hide resolved
filepath: "internal/tests/thrift/hyphenated_file.thrift",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
module, err := compile.Compile(tt.filepath)
if tt.compileError != "" {
require.Error(t, err)
assert.Contains(t, err.Error(), tt.compileError)
return
}
require.NoError(t, err)
require.NotNil(t, module)

opt := &Options{
OutputDir: outputDir,
PackagePrefix: "go.uber.org/thriftrw/gen",
ThriftRoot: thriftRoot,
}
err = Generate(module, opt)
if tt.generateError != "" {
require.Error(t, err, "expected code generation with filepath %v to fail", tt.filepath)
assert.Contains(t, err.Error(), tt.generateError)
} else {
assert.NoError(t, err)
}
})
}
}

func TestGenerate(t *testing.T) {
var (
ts compile.TypeSpec = &compile.TypedefSpec{
Expand Down
2 changes: 1 addition & 1 deletion gen/internal/tests/Makefile
Original file line number Diff line number Diff line change
Expand 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.

173 changes: 173 additions & 0 deletions gen/internal/tests/hyphenated-file/hyphenated-file.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading