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

expose ParseQuery to parse query struct #17

Merged
merged 7 commits into from
Aug 1, 2019
Merged

expose ParseQuery to parse query struct #17

merged 7 commits into from
Aug 1, 2019

Conversation

ashtonian
Copy link
Contributor

Just exposing a parse function that accepts the Query object to allow for greater control. Primary use case is to allow cleaner web framework integration and allow flatter query parameters ie ?query={limit=..,offset=..} vs ?limit=..&offset=..&filter={..}.

On the web framework side this allows the framework to handle things like defaults and validation so that it can be consistent with the rest of the web project. Depending on the web framework (gin,goa) this may also allow it to auto document the structure better.

Copy link
Owner

@a8m a8m left a comment

Choose a reason for hiding this comment

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

The suggestion sounds fair to me, and I'm in favor it.

But one little comment - let's remove the recovering logic from the Parse function, and leave it only in ParseQuery.

rql.go Outdated Show resolved Hide resolved
rql.go Outdated Show resolved Hide resolved
rql.go Outdated Show resolved Hide resolved
ashtonian and others added 4 commits July 24, 2019 14:22
Co-Authored-By: Ariel Mashraki <[email protected]>
Co-Authored-By: Ariel Mashraki <[email protected]>
Co-Authored-By: Ariel Mashraki <[email protected]>
@ashtonian
Copy link
Contributor Author

sweet, thanks for the feedback.

@a8m a8m merged commit f662c28 into a8m:master Aug 1, 2019
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.

2 participants