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 contain multiple values for a param #29

Merged
merged 2 commits into from
Mar 29, 2016
Merged

Allow query strings to contain multiple values for a param #29

merged 2 commits into from
Mar 29, 2016

Conversation

brentonannan
Copy link
Contributor

Twilio requires that multiple StatusCallbackEvent params be passed as separate params; the current version of to_query_string strips multiple value for params by mapping them into a Map, which does not allow multiple values for a key.

As a sidenote, some of the doctests do not pass.

Twilio requires that multiple StatusCallbackEvent params be passed as
separate params; the current version of to_query_string strips multiple
value for params by mapping them into a Map, which does not allow
multiple values for a key.
@danielberkompas
Copy link
Owner

@brentonannan which doctests don't pass?

@brentonannan
Copy link
Contributor Author

I get the same output on both master and my branch:

  1) test doc at ExTwilio.UrlGenerator.build_url/3 (4) (ExTwilio.UrlGeneratorTest)
     test/ex_twilio/url_generator_test.exs:12
     Doctest failed
     code: ExTwilio.UrlGenerator.build_url(Resource, 1) === "https://api.twilio.com/2010-04-01/Accounts//Resources/1.json"
     lhs:  "https://api.twilio.com/2010-04-01/Resources/1.json"
     stacktrace:
       lib/ex_twilio/url_generator.ex:28: ExTwilio.UrlGenerator (module)

.

  2) test doc at ExTwilio.UrlGenerator.build_url/3 (1) (ExTwilio.UrlGeneratorTest)
     test/ex_twilio/url_generator_test.exs:12
     Doctest failed
     code: ExTwilio.UrlGenerator.build_url(Resource) === "https://api.twilio.com/2010-04-01/Accounts//Resources.json"
     lhs:  "https://api.twilio.com/2010-04-01/Resources.json"
     stacktrace:
       lib/ex_twilio/url_generator.ex:19: ExTwilio.UrlGenerator (module)



  3) test doc at ExTwilio.UrlGenerator.build_url/3 (5) (ExTwilio.UrlGeneratorTest)
     test/ex_twilio/url_generator_test.exs:12
     Doctest failed
     code: ExTwilio.UrlGenerator.build_url(Resource, nil, page: 20) === "https://api.twilio.com/2010-04-01/Accounts//Resources.json?Page=20"
     lhs:  "https://api.twilio.com/2010-04-01/Resources.json?Page=20"
     stacktrace:
       lib/ex_twilio/url_generator.ex:31: ExTwilio.UrlGenerator (module)

....

  4) test doc at ExTwilio.UrlGenerator.build_url/3 (6) (ExTwilio.UrlGeneratorTest)
     test/ex_twilio/url_generator_test.exs:12
     Doctest failed
     code: ExTwilio.UrlGenerator.build_url(Resource, nil, iso_country_code: "US", type: "Mobile", page: 20) === "https://api.twilio.com/2010-04-01/Accounts//Resources/US/Mobile.json?Page=20"
     lhs:  "https://api.twilio.com/2010-04-01/Resources/US/Mobile.json?Page=20"
     stacktrace:
       lib/ex_twilio/url_generator.ex:34: ExTwilio.UrlGenerator (module)

.................

Finished in 1.7 seconds (0.9s on load, 0.8s on tests)
46 tests, 4 failures

Randomized with seed 447650

It's odd that it passes CI - does the run include doctests? Could be that I'm running erlang/elixir 18/1.2.3, whereas CI seems to be 17.4/1.0.0.

@danielberkompas
Copy link
Owner

Sorry that this has taken me so long to look at. Reviewing now.

@danielberkompas
Copy link
Owner

@brentonannan Here's what I'm running on my machine:

Erlang/OTP 18 [erts-7.2.1] [source] [64-bit] [smp:4:4] [async-threads:10] [hipe] [kernel-poll:false] [dtrace]

Elixir 1.2.3

Doctests are working for me on master.

..............................................

Finished in 2.1 seconds (0.5s on load, 1.5s on tests)
46 tests, 0 failures

Not sure what is going on.

@danielberkompas
Copy link
Owner

I upgraded the CI: #30
It looks like the doctests pass: https://travis-ci.org/danielberkompas/ex_twilio/builds/119353213

It must be something to do with your environment, because I'm not seeing anything in Travis showing doctests as failing. Maybe you don't have one of the environment variables set? There's an .env.sample file in the repo that shows the environment variables that you need to set in order to develop on the project.

Since I don't see any failures on CI, I'll merge this PR. Thanks!

⭐ ⭐ ⭐

@danielberkompas danielberkompas merged commit 0d373b1 into danielberkompas:master Mar 29, 2016
@danielberkompas
Copy link
Owner

@brentonannan FYI, I published a new release to Hex with this PR included: 0.1.4.

@brentonannan
Copy link
Contributor Author

Cheers @danielberkompas!

Strange about the doctests. Could be locales or something... If I figure it
out at some point, I'll let you know =)
On 30 Mar 2016 7:48 am, "Daniel Berkompas" notifications@github.com wrote:

@brentonannan https://github.com/brentonannan FYI, I published a new
release to Hex with this PR included: 0.1.4.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#29 (comment)

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