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

optional uri queryparams #23

Merged

Conversation

Andersmholmgren
Copy link
Contributor

Justin, this is an initial stab at supporting optional query params for matching.

Once we get to the point where you are happy with it I'll tidy up and squash the commits

@@ -50,6 +50,7 @@ class UriParser extends UriPattern {

final UriTemplate template;
final bool _fragmentPrefixMatching;
final bool _queryParamsAreOptional;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not the best name. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's private and sounds good in an if statement, so it's fine by me

@Andersmholmgren
Copy link
Contributor Author

Fixes #14

@sethladd
Copy link

sethladd commented Jul 8, 2015

cc @justinfagnani

Thanks for the patch!

UriParser(UriTemplate this.template, {bool fragmentPrefixMatching: true})
: _fragmentPrefixMatching = firstNonNull(fragmentPrefixMatching, true) {
UriParser(UriTemplate this.template, {bool fragmentPrefixMatching: true,
bool queryParamsAreOptional: false})
Copy link
Contributor

Choose a reason for hiding this comment

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

style nit: when wrapping named parameters I like to just put every one on a new line

@justinfagnani
Copy link
Contributor

Looks good. Can you squash the commits?

Thanks!

@Andersmholmgren Andersmholmgren force-pushed the feature/fix_uri_queryparams branch from a0b49a0 to ceeb21b Compare July 18, 2015 03:31
@Andersmholmgren
Copy link
Contributor Author

OK PR review changes applied and commit squashed. Please merge and publish to pub

justinfagnani added a commit that referenced this pull request Jul 31, 2015
@justinfagnani justinfagnani merged commit b2749e8 into google:master Jul 31, 2015
@sethladd
Copy link

Thanks!

@donny-dont
Copy link

Great work! Don't forget to close out #14 @sethladd or @justinfagnani

@Andersmholmgren
Copy link
Contributor Author

thanks @justinfagnani

Please publish to pub

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.

4 participants