Add dart_package option #298
Conversation
|
I'm not entirely sure that this is good enough. |
|
|
||
| FileGenerator(this.descriptor, this.options) | ||
| : protoFileUri = calculateUri(descriptor) { | ||
| // if (protoFileUri.isAbsolute) { |
|
Sigurd, |
| if (descriptor.options.hasExtension(Dart_options.dartPackage)) { | ||
| String dartPackage = | ||
| descriptor.options.getExtension(Dart_options.dartPackage); | ||
| return Uri(scheme: 'package', path: '${dartPackage}/${descriptor.name}'); |
There was a problem hiding this comment.
Hey @sigurdm I've been having a look at this, and I think this should be:
return Uri(scheme: 'package', path: '${dartPackage}/${path.basename(descriptor.name)}');descriptor.name returns the full path of the proto file.
In my project the proto root is not the same as the package directory, so I get something like:
dartPackage: "foo/bar"
descriptor.name: "bar/baz.proto"
Combining them together gives "foo/bar/bar/baz.proto" when it should be "foo/bar/baz.proto".
I believe it would be better to ignore the path from the descriptor because the dart package name should always point directly to the directory that contains the dart source file. Right?
There was a problem hiding this comment.
I believe the dart package should not have slashes. It should just be a single name (the name of the package where the proto belongs)
There was a problem hiding this comment.
ooh interesting... That would be tricky in my project... I have one directory structure with several dart packages, and they all share a common proto root.
There was a problem hiding this comment.
Yeah it seems like an unnecessary restriction that your proto file structure should be the same as your dart package structure. Let me explain my project structure:
+ project root
+ server (go packages)
+ main
+ repo1
+ repo2
+ ...
+ tests (go tests)
+ lib (main dart package)
+ repositories (other dart packages)
+ repo1
+ repo2
+ ...
+ proto (proto root)
+ main (proto files for main)
+ repo1 (proto files for repo1)
+ repo2 (proto files for repo2)
+ ...
+ test (dart tests)
It's convenient to have the proto files all in the same directory structure, but the file structure of the Dart packages doesn't match this. I'm trying to ensure my proto files stay platform independent, so I don't really want to rearrange my proto files to match my Dart directory structure. Nor do I really want to have a separate proto root for each package.
There was a problem hiding this comment.
I think it makes sense to have one proto-root per package, if the protos truly belongs in that package. (At least the generated files have to be moved into lib/ of that package.
There was a problem hiding this comment.
OK I'll have a go at rearranging my project / protoc commands. I'll let you know how I get on!
|
👀 |
|
Any plans for the missing review? |
Should fix: #295