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

new: 'query' option in client constructor and per request options #153

Merged
merged 2 commits into from
Feb 21, 2018

Conversation

DonutEspresso
Copy link
Member

Went to try and fix #134 but could not repro. But added unit tests to make sure this is first class and supported moving fwd.

@coveralls
Copy link

coveralls commented Feb 21, 2018

Coverage Status

Coverage increased (+0.2%) to 86.202% when pulling 74dc8e1 on query into c6fd356 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 86.166% when pulling f447c52 on query into bb65be9 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 86.166% when pulling f447c52 on query into bb65be9 on master.

Copy link
Member

@hekike hekike left a comment

Choose a reason for hiding this comment

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

Can you add a test that covers a situation when both ? in the url and the query object is passed. I think the documentation should also cover this case.

@hekike
Copy link
Member

hekike commented Feb 21, 2018

As we have the parsed url technically we could also merge the query objects. Not saying we should, just make sure the expected behavior is documented.

@DonutEspresso
Copy link
Member Author

That's a good point, on client constructor we actually don't take default query options, but no reason we couldn't. If anything, it might be more consistent if we do so as it matches behavior of other options. I'll make the change.

@DonutEspresso
Copy link
Member Author

Updated! Per request values override default values. While merging query params seems useful in some scenarios, I think it's easier to override and push any custom merging logic into userland.

@DonutEspresso DonutEspresso changed the title chore: add unit tests and docs for 'query' option new: 'query' option in client constructor and per request options Feb 21, 2018
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.

How to use query strings?
4 participants