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

Allow query strings to have nested params and arrays. #55

Merged
merged 4 commits into from
Apr 12, 2017

Conversation

doughsay
Copy link
Collaborator

@doughsay doughsay commented Apr 8, 2017

Currently, maxwell can't make requests to servers that allow arrays and nested params in query strings. Phoenix and Rails servers (for example) all support this kind of query string, so not being able to make these kinds of requests is a big blocker for some use-cases.

To implement this, I've copied the query string encoding/decoding module from Plug, because there is no other package currently available that has this functionality. I've opened an issue with the Plug team about it and they are ok with copy-pasting for now. (elixir-plug/plug#539)

Let me know what you think!

(also, use ?w=1 to see the important changes, since I fixed a few unrelated whitespace issues.)

@coveralls
Copy link

coveralls commented Apr 8, 2017

Coverage Status

Coverage decreased (-1.7%) to 98.333% when pulling 79a24c0 on doughsay:master into 2f9e80a on zhongwencool:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.7%) to 98.333% when pulling 79a24c0 on doughsay:master into 2f9e80a on zhongwencool:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.7%) to 98.333% when pulling 79a24c0 on doughsay:master into 2f9e80a on zhongwencool:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.7%) to 98.333% when pulling 79a24c0 on doughsay:master into 2f9e80a on zhongwencool:master.

@doughsay
Copy link
Collaborator Author

doughsay commented Apr 8, 2017

@zhongwencool there are 6 failing ibrowse and httpc adapter tests that were failing on a clean clone from master. My changes shouldn't have changed anything to do with them... Can you take a look when you have a chance?

@zhongwencool
Copy link
Owner

That's great!
Thanks @doughsay
I will fix the failed cases tomorrow, and merge this PR.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.7%) to 98.333% when pulling 6916108 on doughsay:master into 2f9e80a on zhongwencool:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.7%) to 98.333% when pulling 6916108 on doughsay:master into 2f9e80a on zhongwencool:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.7%) to 98.333% when pulling 6916108 on doughsay:master into 2f9e80a on zhongwencool:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.7%) to 98.333% when pulling 6916108 on doughsay:master into 2f9e80a on zhongwencool:master.

@doughsay
Copy link
Collaborator Author

@zhongwencool I noticed some coverage was missing from the Query module, so I also added the tests from Plug, but it seems those tests aren't running when you do mix test. Manually running the tests (mix test test/maxwell/query.exs) works though...

@bitwalker
Copy link
Collaborator

The test file needs to be named query_test.exs for Mix to pick it up.

@doughsay
Copy link
Collaborator Author

Oh, derp! I feel silly now...

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 99.63% when pulling caf297f on doughsay:master into 2f9e80a on zhongwencool:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 99.63% when pulling caf297f on doughsay:master into 2f9e80a on zhongwencool:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 99.63% when pulling caf297f on doughsay:master into 2f9e80a on zhongwencool:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 99.63% when pulling caf297f on doughsay:master into 2f9e80a on zhongwencool:master.

@zhongwencool zhongwencool merged commit 34796e6 into zhongwencool:master Apr 12, 2017
@zhongwencool
Copy link
Owner

Thanks @doughsay :0

@doughsay doughsay mentioned this pull request May 11, 2017
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