-
-
Notifications
You must be signed in to change notification settings - Fork 213
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
Stef negate matcher #134
Stef negate matcher #134
Conversation
Codecov Report
@@ Coverage Diff @@
## master #134 +/- ##
=======================================
+ Coverage 67.09% 68% +0.9%
=======================================
Files 79 80 +1
Lines 2875 2963 +88
Branches 390 394 +4
=======================================
+ Hits 1929 2015 +86
- Misses 728 731 +3
+ Partials 218 217 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done some testing and it behaves as I would expect. Couple of minor things in comments but i think this is in good shape.
.WithHeader("x", "1", MatchBehaviour.RejectOnMatch) | ||
.UsingAnyMethod()) | ||
.RespondWith(Response.Create() | ||
.WithBody(@"{ ""result"": ""reject""}")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we make this example more realistic - like the absense of an API key header returns 401 unauthorised?
var jtoken = JToken.Parse(input); | ||
match = IsMatch(jtoken); | ||
} | ||
catch (Exception) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to catch the exact exception? Wouldn't want to inadvertently catch a SystemOutOfMemoryException for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'll fix this.
/// </returns> | ||
IRequestBuilder UsingAnyVerb(); | ||
/// <returns>The <see cref="IRequestBuilder"/>.</returns> | ||
IRequestBuilder UsingAnyMethod(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this must be a breaking public API change? Worth obsoleting and reinstating the old one? Of following SemVer with the versioning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add these again with [Obsolete]
.
@@ -20,23 +20,24 @@ public static IMatcher Map([CanBeNull] MatcherModel matcher) | |||
string matcherType = parts.Length > 1 ? parts[1] : null; | |||
|
|||
string[] patterns = matcher.Patterns ?? new[] { matcher.Pattern }; | |||
MatchBehaviour matchBehaviour = matcher.RejectOnMatch == true ? MatchBehaviour.RejectOnMatch : MatchBehaviour.AcceptOnMatch; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor but unnecessary comparison with true here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's needed because RejectOnMatch
is a nullable bool.
.UsingAnyMethod()) | ||
.RespondWith(Response.Create() | ||
.WithStatusCode(HttpStatusCode.Unauthorized) | ||
.WithBody(@"{ ""result"": ""api-key missing""}")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tweaked this example a bit - think it makes more sense now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
Thanks for code review. and your updates. I'll merge this to main now. |
No description provided.