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

Projects
None yet
3 participants
@fedefernandez
Copy link
Collaborator

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

This comment has been minimized.

Copy link

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 Apr 22, 2019

@rafaparadela
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 23, 2019

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

3 checks passed

codecov/patch Coverage not affected when comparing 76afadd...48fef20
Details
codecov/project 82.46% (+0.1%) compared to 76afadd
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@fedefernandez fedefernandez deleted the ff/idiomatic-grpc-urls branch Apr 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.