Skip to content
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

feat: whenMatch - for more advanced matching needs #23

Merged
merged 4 commits into from
Apr 11, 2023
Merged

Conversation

samandmoore
Copy link
Member

📰 Summary of changes

What is the new functionality added in this PR?

sometimes, you want to define your own matching logic and now you can. if you just want to match when a certain request parameter is present or if the http method is GET and the first letter of the path is "z". who knows what you'll need? the sky is now the limit.

breaking change: removed CharlatanHttpRequest#pathParameters. it is no longer possible to support this attribute of the request type because the request is provided in the matching phase when we don't know what the path template is and whether its a match yet.

🧪 Testing done

What testing was added to cover the functionality added in this PR

Updated the test suite according to the changes, but existing tests all pass!

sometimes, you want to define your own matching logic and now you can.
if you just want to match when a certain request parameter is present or
if the http method is GET and the first letter of the path is "z". who
knows what you'll need? the sky is now the limit.

breaking change: removed CharlatanHttpRequest#pathParameters. it is no
longer possible to support this attribute of the request type because
the request is provided in the matching phase when we don't know what
the path template is and whether its a match yet.
@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.56 🎉

Comparison is base (4a8b5bc) 95.34% compared to head (eac9840) 95.91%.

❗ Current head eac9840 differs from pull request most recent head d48e7ca. Consider uploading reports for the commit d48e7ca to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #23      +/-   ##
==========================================
+ Coverage   95.34%   95.91%   +0.56%     
==========================================
  Files           4        4              
  Lines          86       98      +12     
==========================================
+ Hits           82       94      +12     
  Misses          4        4              
Impacted Files Coverage Δ
lib/charlatan.dart 100.00% <ø> (ø)
lib/src/charlatan.dart 100.00% <100.00%> (ø)
lib/src/charlatan_http_client_adapter.dart 87.87% <100.00%> (-1.32%) ⬇️
lib/src/charlatan_response_definition.dart 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

),
);
}

Copy link
Member Author

@samandmoore samandmoore Apr 6, 2023

Choose a reason for hiding this comment

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

for some more understanding of where this is valuable...

now we can support matching against graphql API calls like this:

c.whenMatch(
  (r) => r.path == '/api/graphql' && r.body.asJson['queryName'] == 'GetUserQuery',
  (r) => {'name': 'bilbo'}, // but obviously in the shape of GQL responses
);

which we can then wrap in some helpers to look more like this:

c.whenMatch(
  requestMatchesGqlQuery('GetUserQuery'),
  requestGqlResponse({'name': 'bilbo'}),
);

@@ -75,62 +117,33 @@ class Charlatan {
CharlatanResponseBuilder responseBuilder, {
int statusCode = 200,
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we could make a breaking change to make things more typesafe

whenDelete(
  '/users',
  charlatanResponseWith({'id': 123}, status: 200),
);

where charlatanResponseWith returns a CharlatanResponseBuilder that produces a json response with a 200 status code. then if you wanna do something fancier, you drop down to defining your own builder but we change it to require the builder to return a CharlatanHttpResponse instead of Object?. as a reminder, the reason it expects Object? right now is to support returning just a Map from the builder in the normal case where you want a 200 with a json body.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is definitely not a do it in this PR thing, but it might be a nice thing to consider as a better solution to the default status code and short-hand response builder definition.

@samandmoore samandmoore marked this pull request as ready for review April 10, 2023 21:43
@samandmoore samandmoore requested a review from a team as a code owner April 10, 2023 21:43
test('it provides the path parameters to the response builder', () async {
charlatan.whenGet(
'/users/{id}/{other}',
(request) => {'pathParameters': request.pathParameters},
Copy link
Member Author

Choose a reason for hiding this comment

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

If we really want to keep this functionality, I think it's possible. It's just more effort than I wanted to invest here. I think the approach I'd take is to create a subclass of the CharlatanHttpRequest type that can be enhanced with the matched CharlatanResponseDefinition and it can use the underlying dio request and the definition to extract the path parameters. I started to implement that and got hung up on... something... that I can't remember.

I thought this was a cute feature when I added it originally, and I still really like it, but I didn't want to work out the kinks of making it continue to work. Open to someone tackling it though if we can find a clean solution. I just don't want to carry around a gnarly implementation for a feature that I'm not sure folks are clamoring for.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we likely are one of the heaviest users of Charlatan and have zero usages of that API (that I can find at least), I'd agree let's not force it now. If it ends up being missed, we can investigate supporting it again in a future version and folks that really need it can wait for that version.

btrautmann
btrautmann previously approved these changes Apr 11, 2023
Copy link
Contributor

@btrautmann btrautmann left a comment

Choose a reason for hiding this comment

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

One minor thingy but

domain lgtm

example/main.dart Show resolved Hide resolved
test('it provides the path parameters to the response builder', () async {
charlatan.whenGet(
'/users/{id}/{other}',
(request) => {'pathParameters': request.pathParameters},
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we likely are one of the heaviest users of Charlatan and have zero usages of that API (that I can find at least), I'd agree let's not force it now. If it ends up being missed, we can investigate supporting it again in a future version and folks that really need it can wait for that version.

Copy link
Contributor

@btrautmann btrautmann left a comment

Choose a reason for hiding this comment

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

domain lgtm
platform lgtm

@samandmoore samandmoore merged commit ca68622 into main Apr 11, 2023
@samandmoore samandmoore deleted the sam-when-match branch April 11, 2023 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants