Skip to content
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

Allows specifying the namespace and the capitalize params #601

Merged
merged 1 commit into from
Apr 23, 2019

Conversation

fedefernandez
Copy link
Contributor

@fedefernandez fedefernandez commented Apr 22, 2019

Relates to #599

This can be considered the first step where we allow to specify a namespace and the ability to capitalize the gRPC method

@codecov
Copy link

codecov bot commented Apr 22, 2019

Codecov Report

Merging #601 into master will increase coverage by 0.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #601     +/-   ##
=========================================
+ Coverage   82.36%   82.46%   +0.1%     
=========================================
  Files          62       62             
  Lines         964      964             
  Branches       12       12             
=========================================
+ Hits          794      795      +1     
+ Misses        170      169      -1
Impacted Files Coverage Δ
...igherkindness/mu/rpc/internal/server/package.scala 100% <0%> (+8.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76afadd...48fef20. Read the comment docs.

@fedefernandez fedefernandez marked this pull request as ready for review April 22, 2019 21:44
Copy link
Member

@rafaparadela rafaparadela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's great to provide flexibility to define customized protocols through the @service annotation.

I have seen that your tests are proving that both MethodName and Namespace operate perfectly separately, but given that we are transforming the name of the service by capitalizing and concatenating the namespace, I would suggest to prove that both transformations interoperate as we expect.

Beside this minor (non-blocking) suggestion, this implementations looks great to me.

@fedefernandez
Copy link
Contributor Author

Thanks @rafaparadela. The namespace is for the service and the Capitalize option is for the method, so I prefer to have those separated in the annotation. I'll provide only one setting key at plugin level, because as you said they are related.

@fedefernandez fedefernandez merged commit 72c2249 into master Apr 23, 2019
@fedefernandez fedefernandez deleted the ff/idiomatic-grpc-urls branch April 23, 2019 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants