-
Notifications
You must be signed in to change notification settings - Fork 0
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
Unify names: stubs, services, clients, behaviours #4
base: main
Are you sure you want to change the base?
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.
Looks good! Minor comments that can be followed up separately 👍
@@ -54,18 +53,6 @@ defmodule RPCProtoGenHelpers.CLI do | |||
files = | |||
request.proto_file | |||
|> Stream.filter(&rpc_service?/1) |
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.
Suggest we get rid of this filter, can update build_service_metadata
to handle an empty list of service
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.
Yes, let's do this in separate PR. It may cause issues with external (non-Fresha) services, not that this is this plugin's responsibility, but may need adjustments to workflows/scripts
lib/rpc_proto_gen_helpers/cli.ex
Outdated
|
||
behaviour_path = Path.join(package_components ++ ["#{service_name}_client_behaviour.ex"]) | ||
impl_path = Path.join(package_components ++ ["#{service_name}_client_impl.ex"]) | ||
impl_path = Path.join(package_components ++ ["#{service_name_to_snake}_client_impl.ex"]) |
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.
Would be nice to use the proto
file name for this. Just remove the .proto
suffixt nd add _client_impl.ex
! Means we can get rid of the logic for service_name_to_snake
as well, which is nice!
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.
Looking at the buf.example.json
I guess we could just use the name
and do what you suggested
"name": "rpc/gift_cards/v1/gift_cards_service.proto",
would work for the behaviour path as well.
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.
Yes! name
is the filepath for that proto (See: FileDescriptorProto
)
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.
Yeah I am just learning the structure of this as I do the review. 😄
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 actually think we should use the service name for reasons you pointed out previously @CGA1123. If there are multiple services defined per file, then each one should its own impl file mapping to one module.
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.
What would be the advantage of using the file path name?
Keeping things as simple and consistent as possible in my mind. It's the prevalent pattern in protoc plugins (as far as I'm aware) to generate into same path as the proto file was defined in, with a different suffix.
I'd be okay with having multiple modules for different services in the same file, the grpc generator for elixir does exactly that!
If we want a different file per-service, I think at least the base directory path should match the definition base directory path.
In practice, this should always match the package_components
, because we should be defining protos in a directory hierarchy that matches the package hierarchy! But I don't think we should make this assumption at this layer!
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.
Ok, I feel convinced by this. Let's change it to use service file path name.
Yes - we shouldn't enforce any rules at this level, it's up to services to follow conventions.
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.
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.
Sweet. Should package_components
be Path.dirname(file_path)
as well?
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 guess they are equivalent, we'd still need to split (on slashes not dots):
file_path |> Path.dirname |> Path.split
This PR simplifies the plugin by using standard stub names for the RPC client.
It also fixes the filter by not crashing and by specifically selecting a single RPCService.