Feature/retryablehttp #40
Conversation
| @@ -30,7 +30,7 @@ describe('A Retryer', () => { | |||
|
|
|||
| retryableMock.attempt = () => Promise.resolve(true); | |||
|
|
|||
| const retryer = new Retryer(statsDMock, retryableMock, 2, 1, 1, 1); | |||
| const retryer = new Retryer(statsDMock, retryableMock, 2, 1, 1, 1, ['service:intershop']); | |||
There was a problem hiding this comment.
It's not very readable to have arguments like this in a constructor. As a consumer of this code, I can't see what it does, what does 2,1,1,1 do?
Maybe have a look at using named parameters in this constructor? I think Typescript has a way to do this, would result in something like:
const retryer = new Retryer({statsD: statsDMock, retryable: retryableMock, maxAttempts: 2, minInterval: 1, maxInterval: 1, jitter: 1, defaultTags: ['service:intershop']});I've seen that you can also set defaults in the constructor so you won't have to specify all of the arguments. I think most consumers of this constructor would be fine with having the intervals and jitter set to 1 by default
There was a problem hiding this comment.
Thanks, I agree
Typescript does not support "real" named parameters in the constructor:
microsoft/TypeScript#467
When using an object like the example above, you can't use default constructor values anymore. I will play around looking nice solution.
There was a problem hiding this comment.
Also, maybe webstorm is making me lazy :)
It adds the parameter name to these values.
|
|
||
| delete tags.body; | ||
|
|
||
| if (this.requestResult) { |
There was a problem hiding this comment.
You could refactor the tags method into less responsibilities. Perhaps adding a (private) validateRequestResult method underneath
There was a problem hiding this comment.
Should still pass the tests of course
|
|
||
| private isRetryable(requestResult: HTTPRequestResponse | Error): boolean { | ||
| if (requestResult instanceof Error) { | ||
| return requestResult.message !== 'Error: ESOCKETTIMEDOUT' && |
There was a problem hiding this comment.
This is hard to read. Though I understand it's nice to do it in one go, some in between constants would make it more readable fore the next guy updating it
No description provided.