Return-Path: X-Original-To: apmail-cloudstack-dev-archive@www.apache.org Delivered-To: apmail-cloudstack-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 648D110C83 for ; Wed, 24 Jul 2013 17:19:47 +0000 (UTC) Received: (qmail 72846 invoked by uid 500); 24 Jul 2013 17:19:46 -0000 Delivered-To: apmail-cloudstack-dev-archive@cloudstack.apache.org Received: (qmail 72813 invoked by uid 500); 24 Jul 2013 17:19:46 -0000 Mailing-List: contact dev-help@cloudstack.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@cloudstack.apache.org Delivered-To: mailing list dev@cloudstack.apache.org Received: (qmail 72805 invoked by uid 500); 24 Jul 2013 17:19:46 -0000 Delivered-To: apmail-incubator-cloudstack-dev@incubator.apache.org Received: (qmail 72802 invoked by uid 99); 24 Jul 2013 17:19:46 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 24 Jul 2013 17:19:46 +0000 X-ASF-Spam-Status: No, hits=1.5 required=5.0 tests=HTML_MESSAGE,RCVD_IN_DNSWL_LOW,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (nike.apache.org: domain of Alex.Huang@citrix.com designates 66.165.176.63 as permitted sender) Received: from [66.165.176.63] (HELO SMTP02.CITRIX.COM) (66.165.176.63) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 24 Jul 2013 17:19:40 +0000 X-IronPort-AV: E=Sophos;i="4.89,736,1367971200"; d="scan'208,217";a="37159726" Received: from sjcpex01cl01.citrite.net ([10.216.14.143]) by FTLPIPO02.CITRIX.COM with ESMTP/TLS/AES128-SHA; 24 Jul 2013 17:19:17 +0000 Received: from SJCPEX01CL03.citrite.net ([169.254.3.42]) by SJCPEX01CL01.citrite.net ([10.216.14.143]) with mapi id 14.02.0342.004; Wed, 24 Jul 2013 10:19:16 -0700 From: Alex Huang To: Daan Hoogland CC: cloudstack Subject: RE: Review Request 12685: CLOUDSTACK-1532 added utility functions to scan URIs Thread-Topic: Review Request 12685: CLOUDSTACK-1532 added utility functions to scan URIs Thread-Index: AQHOgwTMw4Qf/vWIy0St6yNy33M9AZlsmVwAgAE8yYD//8EZIIAB4XGA///QNXCAAHawAIAEXClg Date: Wed, 24 Jul 2013 17:19:16 +0000 Message-ID: <20CF38CB4385CE4D9D1558D52A0FC058084006@SJCPEX01CL03.citrite.net> References: <20130717154621.1154.91771@reviews.apache.org> <20130719153124.12002.97573@reviews.apache.org> <20CF38CB4385CE4D9D1558D52A0FC05807B137@SJCPEX01CL03.citrite.net> <20CF38CB4385CE4D9D1558D52A0FC05807ED6E@SJCPEX01CL03.citrite.net> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.216.48.12] Content-Type: multipart/alternative; boundary="_000_20CF38CB4385CE4D9D1558D52A0FC058084006SJCPEX01CL03citri_" MIME-Version: 1.0 X-Virus-Checked: Checked by ClamAV on apache.org --_000_20CF38CB4385CE4D9D1558D52A0FC058084006SJCPEX01CL03citri_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 havi= ng 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 Undecid= ed. I'm more in this school. I think it should ask the question "should i= t 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 wr= ite ;) 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 > wrote: Daan, I'm not sure I understand what you're saying here. --Alex From: Daan Hoogland [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 t= hen? On Sat, Jul 20, 2013 at 3:42 PM, Alex Huang > wrote: Daan, It should assert and throw a CloudRuntimeException. https://cwiki.apache.org/confluence/display/CLOUDSTACK/Exceptions+and+loggi= ng --Alex From: Daan Hoogland [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 s= omething? or maybe add an extra 'UnDefined'? On Fri, Jul 19, 2013 at 5:31 PM, Alex Huang > 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 i= s 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 impleme= nt 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 --_000_20CF38CB4385CE4D9D1558D52A0FC058084006SJCPEX01CL03citri_--