Ok, I don't like changing this enum. I'd rather throw it out and start over but you are answering the question by sharing your views on school of programming, i think. BroadcastDomainType by its name implies it could be unknown but never undecided. Do you agree?
I can comply with any school. Op 24 jul. 2013 19:19 schreef "Alex Huang" <alex.hu...@citrix.com> het volgende: > 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.hoogl...@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.hu...@citrix.com> wrote: > **** > > Daan,**** > > **** > > I’m not sure I understand what you’re saying here.**** > > **** > > --Alex**** > > **** > > *From:* Daan Hoogland [mailto:daan.hoogl...@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.hu...@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.hoogl...@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.hu...@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/>**** > > **** > > **** > > ** ** >
