Skip to content

feat: add EchoRequester to java APIC-257 #90

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

Merged
merged 10 commits into from
Jan 20, 2022
Merged

feat: add EchoRequester to java APIC-257 #90

merged 10 commits into from
Jan 20, 2022

Conversation

millotp
Copy link
Collaborator

@millotp millotp commented Jan 14, 2022

🧭 What and Why

🎟 JIRA Ticket: APIC-257

Add the echo requester to the java client, it works by extending the normal response type and sending the echo response depending on the Requester.

The linter wasn't running properly before, this PR fixes all that.

Changes included:

  • Create echo response for each possible response
  • Can choose requester in the constructor

🧪 Test

CI

@millotp millotp self-assigned this Jan 14, 2022
@millotp millotp force-pushed the feat/java-echo branch 2 times, most recently from 44849d9 to 69b06d2 Compare January 14, 2022 15:54
@millotp millotp changed the title feat: add EchoRequester to java APIC-253 feat: add EchoRequester to java APIC-257 Jan 17, 2022
@millotp millotp requested review from a team, eunjae-lee and shortcuts and removed request for a team January 17, 2022 15:57
* @throws ApiException If fail to serialize the request body object
*/
public Call buildCall(
public Object buildCall(
Copy link
Contributor

Choose a reason for hiding this comment

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

What if EchoRequester returns something that extends Call?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Really good idea ! I tried it in 9631eaf

Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Really nice!

@millotp millotp requested a review from shortcuts January 20, 2022 13:57
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Looks really nice! Some minor questions

@millotp millotp requested a review from shortcuts January 20, 2022 15:20
Comment on lines +33 to +43
public String getPath() {
return request.url().encodedPath();
}

public String getMethod() {
return request.method();
}

public String getBody() {
return parseRequestBody(request);
}
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to have a default interface we can extend? So those methods are not duplicated in every classes

Copy link
Member

@shortcuts shortcuts Jan 20, 2022

Choose a reason for hiding this comment

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

or declare those and the request at the EchoResponse class level 🤔

Copy link
Collaborator Author

@millotp millotp Jan 20, 2022

Choose a reason for hiding this comment

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

Unfortunately Java classes can only extend from 1 class, and for the second point the parent class is not accessible from outside, the method would not be callable on the object itself and at the same time be a subclass of it's Response type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The EchoResponse class acts as a namespace here, everything is static.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is the concept of default method for interface but an interface cannot have fields or constructor, so no access to the request.

Copy link
Member

Choose a reason for hiding this comment

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

and for the second point the parent class is not accessible from outside

I meant the utlity classes would be at the EchoResponse level, and the method class would only override the request value when called but yeah I don't know Java enough to explain my idea D:

Too bad it's not possible

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I get it but it's not possible to override at the same time the response (needed to have transparent type for the user) and the EchoResponse

Copy link
Member

Choose a reason for hiding this comment

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

too bad

@millotp millotp requested a review from shortcuts January 20, 2022 15:50
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Nice!!

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