README and implementation differ in query.paramMap
Closed this issue · 6 comments
Hi, I've downloaded v0.4.6 and I'm using it with the documentation in the README as a reference. However, when I parse a Uri from a string, the types differ. Here's a minimal test case:
"Uri.parse" should "provide paramMap as a Map of String to List of String" in {
val parsed = Uri.parse("/?a=b&c=d")
parsed.query.paramMap should be (Map("a" -> List("b"),
"c" -> List("d")))
}And the failure:
[info] Map(a -> Vector((a,Some(b))), c -> Vector((c,Some(d)))) was not equal to Map(a -> List(b), c -> List(d)) (URLParserTest.scala:31)
I'm going off the types at https://github.com/NET-A-PORTER/scala-uri#get-query-string-parameters. Which is the correct behavior?
The actual behaviour is correct, it's my bad for not updating the README when I changed the behaviour (new behaviour is needed to parse URLs like http://example.com?a&b=c&d )
Looking at it, I think this method as been broken for a long time. Before the change mentioned by @philwill-nap, it was returning a Map[String,List[(String,String)]] has didn't line up with the docs either.
I think it makes sense for this method to return a Map[String,Seq[String]]. It was intended as a convenience method for most use cases (most people don't care about query params with no values, and those who do can drill into the params field instead).
I've pushed a change which behaves as the following:
val parsed = Uri.parse("/?a=b&a=c&d=&e&f&f=g")
parsed.query.paramMap should be (Map(
"a" -> Seq("b", "c"),
"d" -> Seq(""),
"e" -> Seq.empty,
"f" -> Seq("g")
))The case this Map loses information is when a URL contains multiple query parameters with the same name, one or more of which do not have a value (e.g. for param f in the above example, we cannot tell there was an empty f query param in the URL). I think if anyone cares about this case, they can use the params field instead.
Let me know if you think this is reasonable.
Seems reasonable to me.
This looks like a much better solution to me, when will the next release be published?