cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Huang <Alex.Hu...@citrix.com>
Subject RE: Review Request 12685: CLOUDSTACK-1532 added utility functions to scan URIs
Date Wed, 24 Jul 2013 17:19:16 GMT
Daan,

Sorry for the late reply.  Now, it kinda goes into code/design philosophy.  I'll tell you
what mine is.

There's one school of thought that code should cover all bases.  To me having undecided is
in that school because it asks "could it be undecided" and the answer is of course yes because
in cs everything is possible.

The other is to say Undecided is never accepted because the code can never do anything with
that.  Having assert to tell developers that this is just not used in this manner and speaks
louder and more accurate than an Undecided.  I'm more in this school.  I think it should ask
the question "should it be undecided".

--Alex

From: Daan Hoogland [mailto:daan.hoogland@gmail.com]
Sent: Sunday, July 21, 2013 8:37 AM
To: Alex Huang
Cc: Hugo Trippaers; cloudstack
Subject: Re: Review Request 12685: CLOUDSTACK-1532 added utility functions to scan URIs

Your remarks on the assert resembles a comment in the code that I didn't write ;)

But seriously; If the string contains a scheme that is not in the enum, you want a exception
to be thrown instead of UnDecided or UnDefined?
regards,

On Sun, Jul 21, 2013 at 5:32 PM, Alex Huang <Alex.Huang@citrix.com<mailto:Alex.Huang@citrix.com>>
wrote:
Daan,

I'm not sure I understand what you're saying here.

--Alex

From: Daan Hoogland [mailto:daan.hoogland@gmail.com<mailto:daan.hoogland@gmail.com>]
Sent: Sunday, July 21, 2013 4:23 AM

To: Alex Huang
Cc: Hugo Trippaers; cloudstack
Subject: Re: Review Request 12685: CLOUDSTACK-1532 added utility functions to scan URIs

Alex,
I saw you put you comments in the commit as comment...??? why not as code then?

On Sat, Jul 20, 2013 at 3:42 PM, Alex Huang <Alex.Huang@citrix.com<mailto:Alex.Huang@citrix.com>>
wrote:
Daan,

It should assert and throw a CloudRuntimeException.

https://cwiki.apache.org/confluence/display/CLOUDSTACK/Exceptions+and+logging

--Alex

From: Daan Hoogland [mailto:daan.hoogland@gmail.com<mailto:daan.hoogland@gmail.com>]
Sent: Saturday, July 20, 2013 3:25 AM
To: Alex Huang
Cc: Hugo Trippaers; cloudstack
Subject: Re: Review Request 12685: CLOUDSTACK-1532 added utility functions to scan URIs

Alex,
it falls through to 'UnDecided' if not known. Do you mean it should throw something? or maybe
add an extra 'UnDefined'?

On Fri, Jul 19, 2013 at 5:31 PM, Alex Huang <alex.huang@citrix.com<mailto:alex.huang@citrix.com>>
wrote:
This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12685/



Ship it!

master: 2d4464d



One review comment is if you are checking for the colon to see if the value has the schema
already, then shouldn't it also check if the schema matches what's declared?  But that shouldn't
block this review.  The code itself is technically sound.


- Alex Huang


On July 17th, 2013, 3:46 p.m. UTC, daan Hoogland wrote:
Review request for cloudstack and Hugo Trippaers.
By daan Hoogland.

Updated July 17, 2013, 3:46 p.m.
Bugs: CLOUDSTACK-1532
Repository: cloudstack-git
Description

the review for the complete patch was open to long and no longer applicable, so I am starting
with smaller patches as this is really needed to implement CLOUDSTACK-1532


Testing

unit testing


Diffs

  *   api/src/com/cloud/network/Networks.java (5aede05)
  *   api/test/com/cloud/network/NetworksTest.java (PRE-CREATION)

View Diff<https://reviews.apache.org/r/12685/diff/>





Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message