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

Improves how the params are received in the annotation #602

Merged
merged 2 commits into from Apr 23, 2019

Conversation

Projects
None yet
2 participants
@fedefernandez
Copy link
Collaborator

commented Apr 23, 2019

What this does?

Allows specifying named params in the service annotation

Relates to #599

Checklist

  • Reviewed the diff to look for typos, println and format errors.
  • Updated the docs accordingly.

@fedefernandez fedefernandez requested a review from juanpedromoreno Apr 23, 2019

@juanpedromoreno
Copy link
Member

left a comment

Thanks @fedefernandez , out of curiosity, in terms of code generation in the sbt plugin side, do we need to update something related to these changes? I'm guessing that this is part of the WIP to cover #599 so, at some point, we will need to do it.

@codecov

This comment has been minimized.

Copy link

commented Apr 23, 2019

Codecov Report

Merging #602 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #602   +/-   ##
=======================================
  Coverage   82.46%   82.46%           
=======================================
  Files          62       62           
  Lines         964      964           
  Branches       12       12           
=======================================
  Hits          795      795           
  Misses        169      169

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 72c2249...3ff4a0a. Read the comment docs.

@fedefernandez

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 23, 2019

@juanpedromoreno with these changes, the scenario in #599 is covered since it's using the scala file for generating the proto file. See https://github.com/sander/rpctest/compare/master...fedefernandez:ff/update-namespace?expand=1

I would want to create two following PRs. One with a new setting in the idl plugin for adding those annotations params and another one for the docs. Does it make sense?

@juanpedromoreno
Copy link
Member

left a comment

Yeah, it makes sense. Thanks @fedefernandez !

@fedefernandez fedefernandez merged commit 635abd0 into master Apr 23, 2019

4 checks passed

codecov/patch Coverage not affected when comparing 72c2249...3ff4a0a
Details
codecov/project 82.46% remains the same compared to 72c2249
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@fedefernandez fedefernandez deleted the ff/annotation-params 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.