Feature/build integer response #18
Conversation
| return 0; | ||
| errors.add(Error.create("No context", | ||
| "No queue item Location header could be found despite getting a valid HTTP response.", | ||
| "None")); |
There was a problem hiding this comment.
Lets take the Error.create call outside and do something like:
final Error error = Error.create(null,
"No queue item Location header could be found despite getting a valid HTTP response.",
NumberFormatException.class.getCanonicalName());
List<Error> errors = Lists.newArrayList(error);
return IntegerResponse.create(null, errors);
I'd prefer to delay the creation of the list until the last possible minute as this is an exceptional case. In the code further above lets just pass a null like so:
return IntegerResponse.create(Integer.valueOf(matcher.group(1)), null);
| import com.google.common.base.Function; | ||
| import com.google.common.base.Throwables; | ||
| import com.google.common.collect.Lists; | ||
| import com.google.common.io.CharStreams; |
|
Few comments to address but all-in-all it looks good. Thanks for taking this work on. |
| return 0; | ||
| errors.add(Error.create("No context", | ||
| "No queue item Location header could be found despite getting a valid HTTP response.", | ||
| "None")); |
There was a problem hiding this comment.
@cdancy, in creating this error, is it possible to have the original context (the POST)?
There was a problem hiding this comment.
@martinda whatever is in the httpResponse is really all we have to work with. What were you thinknig?
There was a problem hiding this comment.
@cdancy I was thinking of printing the HTTP response, in case it might give a hint to the user.
| import com.google.common.base.Function; | ||
| import com.google.common.base.Throwables; | ||
| import com.google.common.collect.Lists; | ||
| import com.google.common.io.CharStreams; |
| if (url != null) { | ||
| Matcher matcher = pattern.matcher(url); | ||
| if (matcher.find() && matcher.groupCount() == 1) { | ||
| return Integer.valueOf(matcher.group(1)); | ||
| return IntegerResponse.create(Integer.valueOf(matcher.group(1)), errors); |
There was a problem hiding this comment.
In the code further above lets just pass a null like so:
return IntegerResponse.create(Integer.valueOf(matcher.group(1)), null);
I was trying to apply the same approach as in the Bitbucket Rest Understanding Error objects where the error object is never null so it can always be easily checked.
There was a problem hiding this comment.
That's fair but lets let the underlying process take care of that instead of aggressively creating it up front.
There was a problem hiding this comment.
Now I get it, we let IntegerResponse convert null to an empty list.
| import com.google.common.base.Function; | ||
| import com.google.common.base.Throwables; | ||
| import com.google.common.collect.Lists; | ||
| import com.google.common.io.CharStreams; |
martinda
force-pushed
the
feature/build-integer-response
branch
from
795b2d0 to
615f80f
Compare
|
@martinda LGTM. Anything else you want to do here or are we ready to merge? |
|
@cdancy I am ready to merge. |
|
Thanks again @martinda ! |
This PR is to fix issue #16 .
This PR implements a new return type for endpoints returning an integer value. The new return type is
IntegerResponse, and it is an integer encapsulated in an object that can also carries error information.