Hi Dave, I see what you mean by colons wouldn't be allowed. Nothing changed but I went for this solution for consistency (over backward compatibility). I will change again. Let me get to work first. nice working around the globe;)
Daan On Wed, Aug 7, 2013 at 3:57 AM, Sheng Yang <sh...@yasker.org> wrote: > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12849/ > > On August 6th, 2013, 11:40 p.m. UTC, *Sheng Yang* wrote: > > > api/src/com/cloud/network/Networks.java<https://reviews.apache.org/r/12849/diff/5/?file=337785#file337785line135> > (Diff > revision 5) > > public String scheme() { > > 95 > > // do we need to check that value does not contain a scheme > > 135 > > return new URI(scheme,value.toString(),null,null); > > I think by following the previous case, it should be > URI(scheme,value.toString()) rather than "null, null". It possibly doesn't > make difference, but it's better not to use different constructor. > > The same with several URI constructor above. > > And I doubt if it's really need to discard this check. At least they would > working for other broadcast domain type. > > On August 7th, 2013, 1:45 a.m. UTC, *Sheng Yang* wrote: > > Dave explained this. Please drop this issue. > > On August 7th, 2013, 1:54 a.m. UTC, *Dave Cahill* wrote: > > Thanks for the quick reply Sheng! > > Actually, I dug deeper and realized that if we use the 4-parameter URI > constructor, the "value" parameter is interpreted as host, and colons > wouldn't be allowed! > > It looks like the only way to preserve the behavior as it was is to stick > with the old string concatenation behavior of scheme + "://" + value. In > fact, this is what Daan and I agreed on in IRC yesterday, but it looks like > he went with the 4 param constructor - I'll discuss with him later to see > what changed. > > Thank you Dave! > > > - Sheng > > On August 6th, 2013, 2:25 p.m. UTC, daan Hoogland wrote: > Review request for cloudstack, Chiradeep Vittal, Dave Cahill, Koushik > Das, and Sheng Yang. > By daan Hoogland. > > *Updated Aug. 6, 2013, 2:25 p.m.* > *Repository: * cloudstack-git > Description > > Both BroadcastDomainType and IsolationType needed some extra code for > backwards compatibility > > Diffs > > - api/src/com/cloud/network/Networks.java (c76c3d4) > - api/test/com/cloud/network/NetworksTest.java (31114e8) > > View Diff <https://reviews.apache.org/r/12849/diff/> >
