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

Add support for Unit response type in HEAD method requests. #3648

Closed
wants to merge 5 commits into from

Conversation

forrestpangborn
Copy link

@forrestpangborn forrestpangborn commented Oct 28, 2021

This adds support for Unit (additionally to preexisting Void support) response types for HEAD method requests.

throw methodError(method, "HEAD method must use Void as response type.");
if (requestFactory.httpMethod.equals("HEAD")
&& !Void.class.equals(responseType)
&& !Unit.class.equals(responseType)) {
Copy link
Member

@JakeWharton JakeWharton Oct 28, 2021

Choose a reason for hiding this comment

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

This will throw NoClassDefFound if you do not have the Kotlin stdlib as a runtime dep. The conditional needs to account for this either with a try/catch or a boolean with separate lookup mechanism.

Copy link
Author

@forrestpangborn forrestpangborn Oct 28, 2021

Choose a reason for hiding this comment

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

I see a similar case elsewhere in the project doing this here. Would you recommend replicating this or attempting to extract into something reusable?

Copy link
Author

@forrestpangborn forrestpangborn Oct 29, 2021

Choose a reason for hiding this comment

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

I added a static function Utils.isUnitAvailable() - will gladly move elsewhere or can convert to a local try/catch if thats preferable.

@JakeWharton
Copy link
Member

@JakeWharton JakeWharton commented Oct 28, 2021

Also needs tests!

@forrestpangborn
Copy link
Author

@forrestpangborn forrestpangborn commented Jan 29, 2022

@JakeWharton is there anything else you'd recommend or prefer for this?

I'm willing to change the approach it uses for determining if Unit is available at runtime. I can also add more tests if you think thats appropriate and if you can suggest what would also be good to verify.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants