Skip to content

Commit

Permalink
Stop putting directories from the file names onto the import path (#82)
Browse files Browse the repository at this point in the history
This behavior was incorrect and led to strange behaviors. Proto files are meant
to be importable using hierarchical names which should be found relative to the
import paths. Hence, adding the directory part of the given path names to the
import path only encourages incorrect use of hierarchical names

For example, if my proto file is named api.proto and is not meant to be
imported with a hierarchical name, it is incorrect to allow me to specify it as
`path/to/api.proto` because if I import a second file which happens to import
`api.proto` the parser has no way of knowing that it is supposed to be the same
as `path/to/api.proto` which I originally gave it.

Addresses issue #80
  • Loading branch information
ecbaldwin authored and ktr0731 committed Aug 17, 2018
1 parent 10a5376 commit 638748b
Show file tree
Hide file tree
Showing 5 changed files with 5 additions and 27 deletions.
14 changes: 0 additions & 14 deletions adapter/internal/protoparser/parser.go
Original file line number Diff line number Diff line change
@@ -1,27 +1,13 @@
package protoparser

import (
"path/filepath"

"github.com/jhump/protoreflect/desc"
"github.com/jhump/protoreflect/desc/protoparse"
)

func ParseFile(fnames []string, paths []string) ([]*desc.FileDescriptor, error) {
encountered := map[string]bool{}
paths = append(paths, ".")
encountered["."] = true
for _, path := range paths {
encountered[path] = true
}

for _, fname := range fnames {
path := filepath.Dir(fname)
if !encountered[path] {
paths = append(paths, path)
encountered[path] = true
}
}
p := &protoparse.Parser{
ImportPaths: paths,
}
Expand Down
2 changes: 1 addition & 1 deletion adapter/parser/parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
func TestParseFile(t *testing.T) {
t.Run("importing", func(t *testing.T) {
fnames := []string{testdata("importing", "library.proto")}
pkgs, err := ParseFile(fnames, nil)
pkgs, err := ParseFile(fnames, []string{"testdata/importing"})
require.NoError(t, err)
assert.Len(t, pkgs, 1)
assert.Len(t, pkgs[0].Messages, 4)
Expand Down
2 changes: 1 addition & 1 deletion adapter/protobuf/message_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func TestMessage(t *testing.T) {

t.Run("importing", func(t *testing.T) {
libraryProto := testdata("importing", "library.proto")
d, err := protoparser.ParseFile([]string{libraryProto}, nil)
d, err := protoparser.ParseFile([]string{libraryProto}, []string{"testdata/importing"})
require.NoError(t, err)

d = append(d, d[0].GetDependencies()...)
Expand Down
8 changes: 1 addition & 7 deletions di/dependency.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package di

import (
"io"
"path/filepath"
"sync"

"github.com/ktr0731/evans/adapter/gateway"
Expand Down Expand Up @@ -99,12 +98,7 @@ func resolveProtoPaths(cfg *config.Config) ([]string, error) {
return res[0], nil
}

fpaths := make([]string, 0, len(cfg.Default.ProtoFile))
for _, f := range cfg.Default.ProtoFile {
fpaths = append(fpaths, filepath.Dir(f))
}

for _, p := range append(cfg.Default.ProtoPath, fpaths...) {
for _, p := range cfg.Default.ProtoPath {
path, err := parse(p)
if err != nil {
return nil, err
Expand Down
6 changes: 2 additions & 4 deletions di/dependency_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@ func TestInitEnv(t *testing.T) {
cfg.Default.ProtoFile = []string{"./hoge", "./foo/bar"}
paths, err := resolveProtoPaths(cfg)
require.NoError(t, err)
require.Len(t, paths, 2)
require.Exactly(t, []string{".", "foo"}, paths)
require.Len(t, paths, 0)
})

setEnv := func(k, v string) func() {
Expand All @@ -57,8 +56,7 @@ func TestInitEnv(t *testing.T) {

paths, err := resolveProtoPaths(cfg)
require.NoError(t, err)
require.Len(t, paths, 1)
require.Equal(t, "/fuga", paths[0])
require.Len(t, paths, 0)
})

t.Run("error/proto path", func(t *testing.T) {
Expand Down

0 comments on commit 638748b

Please sign in to comment.