> but do want to point out that without this change, url encoding is plain
> broken
TL;DR: The current behaviour urlEncode may look broken to you and may, but it's
clearly deliberate and not a coding error. Since someone with lots of HTTP
experience went to the trouble to add it, I'm not in favour of ripping it out
without understand why it's there or _lots_ of evidence that it's no longer
required.
@diwakergupta Of course I see where you're coming from. It _seems_ broken to me
too. But on the evidence (all tests passing and, until the signature feature
request, no JIRA issues or user group questions) it's working pretty well. I
know it's not the "standard" URI encoding that you and I might expect, but
without understanding the reason for the "non-standard" encoding I'm reluctant
to rip it out.
It's not as though there's an obvious coding bug in there - someone with a
_lot_ of HTTP experience has gone to some lengths to put this behaviour in
there deliberately.
As regards the test case failure: again, I don't know whether the `+` case is
omitted by accident or deliberately. I also don't know what the expected result
_should_ be in that case. Perhaps the test case would be written as
```
assertEquals(uriBuilder("/redirect").addQuery("foo", key).toString(),
"/redirect?foo=" + key,replace('+', ' '));
```
to indicate the _intended_ behaviour of `+` being replaced by ` `?
Back to the immediate use case at hand: can get JCLOUDS-200 working without
this change by encoding the URL first, or is it broken even then? If it can
work after https://github.com/jclouds/jclouds/pull/78, I'd prefer to leave
things as-is for 1.6.2-incubating and get more background on this before
hopefully cleaning up the encoding for 1.6.3-incubating and 1.7.0.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/82#issuecomment-21982414
