Closed a8t3r closed 1 year ago
Hi @a8t3r thanks for the great PR. Why not just import the phasetwo-admin-client
as a test dependency in the pom, rather than generating the java source from the spec? That way, we don't have to keep it up to date in two places when the spec changes.
Otherwise, the updates to OrganizationResourceTest
look great. I agree it makes it much easier to read.
Yeah, firstly I tried with test dependency, but found some drawbacks:
1) method signatures return a specific type (eg List<Organization>
instead of javax.ws.rs.core.Response
) so it's harder to test http statuses and headers. If use test dependency, http statuses can be handled by exceptions, but I have no idea about headers.
2) if you want to create new api, you first need to update the phasetwo-admin-client
contracts.
Good points.
Did you use an openapi-generator
command to generate those *Api
classes? Can you add that to the README so that others can do the same if they make changes?
Otherwise, looks ready to merge.
No, I copypasted them from jar artifact.
I think your point about the only single source of truth is reasonable. Please verify https://github.com/p2-inc/phasetwo-java/pull/1 I rewrited tests with that fixes - looks good.
So before writing new api you need design the contract.
@a8t3r I released your changes to phasetwo-admin-client
as version 0.1.1
. (Please ignore version 0.1
, as I made a mistake setting up the release automation)
@a8t3r Let me know when this is ready to go, and I'll review again and merge.
@xgp I updated dependency version to stable and now it's ready for review.
I copypasted openapi specifications from phasetwo-admin-client. I think it's much easier than
SimpleHttp.Response
Return type for method signatures replaced toResponse
for proper status/headers validation;