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

Fixes asString when query has 1 param with no value #13

Merged
merged 2 commits into from
May 9, 2018

Conversation

albertpastrana
Copy link
Owner

When there was a key with no values in the query string,
there was a implicit conversion from string to list that
made the code compile but that messed up with the params.

That way, this query string ?hello would be translated
into ?h&e&l&l&o.

Signed-off-by: Albert Pastrana albert.pastrana@gmail.com

When there was a key with no values in the query string,
there was a implicit conversion from string to list that
made the code compile but that messed up with the params.

That way, this query string `?hello` would be translated
into `?h&e&l&l&o`.

Signed-off-by: Albert Pastrana <albert.pastrana@gmail.com>
@albertpastrana
Copy link
Owner Author

@nathankleyn it would be great if you can take a look at this one :)

@coveralls
Copy link

coveralls commented May 8, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 8a99a75 on fix-query-asString-bug into 10aad22 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 67cf0b4 on fix-query-asString-bug into 10aad22 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 67cf0b4 on fix-query-asString-bug into 10aad22 on master.

Copy link
Collaborator

@nathankleyn nathankleyn left a comment

Choose a reason for hiding this comment

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

One super quick thing, up to you if you want to fix as it works as it is. 👍

@@ -69,40 +68,41 @@ class URLSpec extends Specification with ScalaCheck with TestData {
"path" >> {
"should decode the rawPath correctly" >> {
"when it has a space" >> {
URL("http://example.com/hello%20world/").map(_.path) must beASuccessfulTry(Some("/hello world/"))
URL("http://example.com/hello%20world/").map(_.path) must beASuccessfulTry(Option("/hello world/"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Think both of these lines should still be Some?

Copy link
Owner Author

Choose a reason for hiding this comment

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

This is because Intellij, if I leave it as Some, then the line appears as a compilation error (even if it's not). So I have to either do what I did or leave as Some and add the type like this: beASuccessfulTry[Option[String]](Some("/hello world/")).

Can't remember where I got the numbers for the exponential
backoff test, but after several intermitent failures I just
checked what the min and max values should be and they are
slightly different from what I got.

With these new values the test should never fail.

Signed-off-by: Albert Pastrana <albert.pastrana@gmail.com>
@codecov-io
Copy link

codecov-io commented May 9, 2018

Codecov Report

Merging #13 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #13   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          11     11           
  Lines         194    194           
  Branches        1      1           
=====================================
  Hits          194    194
Impacted Files Coverage Δ
url/src/main/scala/uscala/net/URL.scala 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10aad22...8a99a75. Read the comment docs.

Copy link
Collaborator

@nathankleyn nathankleyn left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 👏

@albertpastrana albertpastrana merged commit 5c61697 into master May 9, 2018
@albertpastrana albertpastrana deleted the fix-query-asString-bug branch May 9, 2018 09:13
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.

None yet

4 participants