-
Notifications
You must be signed in to change notification settings - Fork 423
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
Add a dev-tool subpackage #2167
Conversation
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.
Could we add some tests?
import ArgumentParser | ||
import Foundation | ||
|
||
struct GenerateJSON: ParsableCommand { |
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.
Should we have both commands take an output
param as well instead of printing to stdout? Not necessary but we could automate things further with a build plugin eventually if we did this instead.
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.
Similar to the other comment, I don't think we need this now: we can just redirect the output to a file using bash etc.
I honestly don't think it's worth it: this is a dev tool and absolutely shouldn't be used by anyone else (which is why it's a separate package). There's also not much to test as the code generator is pretty heavily tested already. I also don't want to set a high bar for this tools package, we should bias towards adding things which make our lives easier and having rough edges is absolutely fine. We can smooth them out as and when that becomes necessary. |
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.
I was just thinking that if we're depending on the output of this package to be correct (so that our tests for the actual gRPC packages are correct) it would be worth to have at least one or two tests - don't think that's necessarily holding this package to a high bar. But that's alright, we can always add them later.
Motivation: A number of test in this package and others rely on ad-hoc services using Codable. This is less overhead than using protobuf as you it's not always available. It also means the messages are defined in Swift so they're easy to change without needing to regenerate. However, the service glue code is hand rolled. We can avoid this by having a little adapter sit on top of the code gen lib. Modifications: - Add a grpc-dev-tool package to dev. We can use this as a place to add tooling and other helpers without worrying about worsening the experience for end users (because of additional dependencies, more public API and so on). - For now this has a single executable for generating code from a JSON config file. The schema for the services is limited, but that's fine, it's not a general purpose tool. Result: - We have a tool which can generate grpc code from a JSON definition which uses Codable message types.
c068617
to
8417a7e
Compare
Motivation:
A number of test in this package and others rely on ad-hoc services using Codable. This is less overhead than using protobuf as you it's not always available. It also means the messages are defined in Swift so they're easy to change without needing to regenerate. However, the service glue code is hand rolled. We can avoid this by having a little adapter sit on top of the code gen lib.
Modifications:
Result: