Skip to content
This repository has been archived by the owner on May 11, 2021. It is now read-only.

Unwrapping ExecutionException when executed synchronously. Fixes #139 #142

Closed
wants to merge 1 commit into from

Conversation

nurkiewicz
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) when pulling c1d0aca on issues/139 into 8a6d251 on master.

executor.exchange(HttpMethod.GET, [host: 'http://localhost:7777', url: '/api'.toURI()], Object)

then:
ResourceAccessException e = thrown(ResourceAccessException)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is one of the examples when IMHO def is readable enough and much more consist. Wouldn't you agree @marcingrzejszczak?

Copy link
Contributor

Choose a reason for hiding this comment

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

def in the method name of test? Of course it does - I also use def in that case

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, the line I commented :).

ResourceAccessException e = thrown(ResourceAccessException)
e.message?.contains('localhost:7777')

vs.

def e = thrown(ResourceAccessException)
e.message?.contains('localhost:7777')

especially that it is used in the next line.

Copy link
Contributor

Choose a reason for hiding this comment

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

:D I'm not an evangelist of anti-def. Everything is ok if you use common sense. If you have such a case then it does make sense to use def :)

@szpak
Copy link
Collaborator

szpak commented Dec 21, 2014

+1

@marcingrzejszczak
Copy link
Contributor

This change is a breaking one? Meaning that instead of throwing our custom exception we will be throwing the original one from RestTemplate?

@nurkiewicz
Copy link
Contributor Author

Everything is fine, this fixes a regression after #13. ServiceRestClient was mistakenly wrapping our custom ResponseException with ExecutionException. This PR restores old behavior, rolling back backward incompatible change I made unintentionally.

@mariuszs
Copy link
Collaborator

+1

@nurkiewicz nurkiewicz self-assigned this Dec 22, 2014
@nurkiewicz
Copy link
Contributor Author

Merged (04ad2c9).

@nurkiewicz nurkiewicz closed this Dec 22, 2014
@nurkiewicz nurkiewicz deleted the issues/139 branch December 30, 2014 18:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants