-
Notifications
You must be signed in to change notification settings - Fork 6
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
Generate code file for protos with no definitions #27
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -98,9 +98,11 @@ package struct ProtobufCodeGenParser { | |
|
||
extension ProtobufCodeGenParser { | ||
fileprivate func codeDependencies(file: FileDescriptor) -> [Dependency] { | ||
var codeDependencies: [Dependency] = [ | ||
Dependency(module: "GRPCProtobuf", accessLevel: .internal) | ||
] | ||
var codeDependencies: [Dependency] = [] | ||
|
||
if file.services.count > 0 { | ||
codeDependencies.append(Dependency(module: "GRPCProtobuf", accessLevel: .internal)) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't quite right: we'll still generate some imports in some cases unnecessarily (i.e. if the file defines a message which depends on a type which is bundled with SwiftProtobuf). We should return the empty array if there are no services. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, good point |
||
|
||
// If there's a dependency on a bundled proto then add the SwiftProtobuf import. | ||
// | ||
|
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
// Leading trivia. | ||
syntax = "proto3"; | ||
|
||
package foo; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@glbrntt note this change. I'm happy to take a different approach than this if you'd rather.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, this is good. We only switched to
exact
for the tagged releases,main
is fine between releases.