feat: Require correct import path #32
Conversation
| expectedPrefix := strings.ReplaceAll(*f.Package, ".", "/") + "/" | ||
| if !strings.HasPrefix(*f.Name, expectedPrefix) { | ||
| panic(fmt.Errorf("file name %s does not start with expected %s; please make sure your folder structure matches the proto files fully-qualified names", *f.Name, expectedPrefix)) | ||
| } |
There was a problem hiding this comment.
Does this seems like a good enough check to you?
There was a problem hiding this comment.
I think so. @testinginprod what do you think?
There was a problem hiding this comment.
looks correct to me. Should we test it on the sdk?
There was a problem hiding this comment.
I did a branch on the SDK where I pointed to this PR, and it worked
| regenerate: | ||
| protoc --gogo_out=Mgoogle/protobuf/descriptor.proto=github.com/cosmos/gogoproto/protoc-gen-gogo/descriptor:../../../../ --proto_path=../../../../:../protobuf/:. *.proto | ||
| cd .. && protoc --gogo_out=Mgoogle/protobuf/descriptor.proto=github.com/cosmos/gogoproto/protoc-gen-gogo/descriptor:../../../ --proto_path=../../../:./protobuf/:. gogoproto/*.proto |
There was a problem hiding this comment.
The fix here is actually just doing some cd so that the OS path matches the FQ name
|
I'll fix the tests, it seems like the check added in #32 (comment) is shouting on some other (incorrect) import paths. |
|
There are a lot of failing tests, |
amaury1093
force-pushed
the
am/correct-import-path
branch
from
165f930
to
97e0165
Compare
amaury1093
force-pushed
the
am/correct-import-path
branch
from
b582d8b
to
5ee00ef
Compare
There was a problem hiding this comment.
I moved all test fixes into a separate PR: #34. There's 200+ files changed, and I'm not finished yet.
I'll continue working on those tests, but for review's sake I think those should be 2 PRs. We can also merge this one first if that's useful (to unblock v0.47).
| expectedPrefix := strings.ReplaceAll(*f.Package, ".", "/") + "/" | ||
| if !strings.HasPrefix(*f.Name, expectedPrefix) { | ||
| panic(fmt.Errorf("file name %s does not start with expected %s; please make sure your folder structure matches the proto files fully-qualified names", *f.Name, expectedPrefix)) | ||
| } |
There was a problem hiding this comment.
I did a branch on the SDK where I pointed to this PR, and it worked
|
If we are okay to merge with failing tests for now, then can an admin do it? I'll fix the tests in #34 |
Fixes the 2 incorrect registrations:
cc @testinginprod (can't find you in the reviewers dropdown)
tests are in a separate PR for now: #34