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

programmatic client generation for MP Rest Client, fixes #1317 #1486

Merged
merged 1 commit into from
Mar 15, 2019

Conversation

michalszynkiewicz
Copy link
Member

No description provided.

@geoand
Copy link
Contributor

geoand commented Mar 14, 2019

@michalszynkiewicz tests are failing

@michalszynkiewicz
Copy link
Member Author

@geoand the tests were fine, a couple unused imports sneaked in

@geoand
Copy link
Contributor

geoand commented Mar 14, 2019

@michalszynkiewicz cool :)

@geoand
Copy link
Contributor

geoand commented Mar 14, 2019

@michalszynkiewicz I assume that when this is merged the quickstart will need to be updated as well?

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Tests should be integrated with the others.

integration-tests/rest-client/pom.xml Outdated Show resolved Hide resolved
@michalszynkiewicz
Copy link
Member Author

@geoand what's currently in the quickstarts will work. They don't cover programmatic creation of the client, I don't know if it's something that should be/is worth adding to the quickstarts

@geoand
Copy link
Contributor

geoand commented Mar 14, 2019

Got it, thanks! Maybe then we could add a section about the improved support you added in the doc about the rest-client?

@michalszynkiewicz
Copy link
Member Author

michalszynkiewicz commented Mar 15, 2019

the native build failed with a timeout on downloading smallrye metrics artifact from the jboss nexus.
I retriggered it.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

@michalszynkiewicz the implementation looks good.

I added 2 very minor comments, could you adjust and I'll merge?

*
* @author Michal Szynkiewicz, michal.l.szynkiewicz@gmail.com
* <br>
* Date: 3/15/19
Copy link
Member

Choose a reason for hiding this comment

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

I would drop that. We don't really use that, we have the git history.


== Further reading

link:https://download.eclipse.org/microprofile/microprofile-rest-client-1.2.1/microprofile-rest-client-1.2.1.html[MicroProfile Rest Client specification]
Copy link
Member

Choose a reason for hiding this comment

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

Very minor nitpicking: even if there is only one element, I would use a list to have a better rendering.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Let's wait for CI to go green.

@gsmet gsmet merged commit 8286023 into quarkusio:master Mar 15, 2019
@gsmet gsmet added this to the 0.12.0 milestone Mar 15, 2019
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