Feat/status matcher #1102
Feat/status matcher #1102
Conversation
| // Non-error response(< 400) | ||
| NonError = 'nonError', | ||
| // Any error response (>= 400) | ||
| Error = 'error', |
There was a problem hiding this comment.
I think these would be better named 2XX, 4XX, etc.
There was a problem hiding this comment.
I had considered that also, I'm glad you mentioned it.
I thought that might be too casual, so I stuck with the definitions in the rust core. Presumably, the first 5 at least were named after the actual classes (e.g. https://developer.mozilla.org/en-US/docs/Web/HTTP/Status).
| this.interaction.withStatus(status); | ||
| willRespondWith( | ||
| status: number | Matcher<number>, | ||
| builder?: V4ResponseBuilderFunc |
There was a problem hiding this comment.
Unrelated, but is the builder pattern the right thing to have in this API? It's not very idomatic.
There was a problem hiding this comment.
I definitely ummed and ahh'd about this. It could probably be a well defined type (or possibly set of types). The issue with using TypeScript types to represent different possible inputs, is that people still use JS and eventually put in garbage, so you need validation on top of it.
The builder pattern doesn't allow it, and saves all of this additional validation code, whilst also still giving you a typed interface.
But I'll revisit before it's no longer behind a flag. Thanks.
| @@ -296,7 +299,7 @@ export class InteractionWithPluginRequest | |||
| status: number, | |||
| builder?: V4PluginResponseBuilderFunc | |||
| ): V4InteractionWithPluginResponse { | |||
| this.interaction.withStatus(status); | |||
| this.interaction.withStatus(reify(status) as number); | |||
There was a problem hiding this comment.
I suggest removing the as number. You could do this by adding a type parameter to reify.
src/v4/http/types.ts
Outdated
| @@ -71,7 +72,7 @@ export interface V4InteractionWithCompleteRequest { | |||
|
|
|||
| export interface V4InteractionwithRequest { | |||
| willRespondWith( | |||
| status: number, | |||
| status: number | Matcher<number>, | |||
There was a problem hiding this comment.
Is this right? Matcher<number> isn't necessarily the same as StatusCodeMatcher<T>.
Also, if these aren't always http requests, then I'm not sure that number is right
There was a problem hiding this comment.
It can't be a non HTTP request, the type-state designed DSL/interface won't allow that.
There was a problem hiding this comment.
...but your other point is correct, thank you.
| @@ -137,7 +138,7 @@ export interface V4InteractionWithPlugin { | |||
|
|
|||
| export interface V4InteractionWithPluginRequest { | |||
There was a problem hiding this comment.
Unrelated, but does this need to be a separate interface?
There was a problem hiding this comment.
It does, as the builder func is different and you must provide certain details prior to this being an allowed state. See the type-state pattern, which I think resolves so many problems with the current DSL (and with the more complicated options allowed in V4, such as plugins).
mefellows
force-pushed
the
feat/status-matcher
branch
2 times, most recently
from
9d8af20 to
5ecb0b4
Compare
mefellows
force-pushed
the
feat/status-matcher
branch
from
5ecb0b4 to
ef5e12e
Compare
Not ready for merging, but FYI.
To be resumed when pact-foundation/pact-reference#296 is completed.