> On Jan. 31, 2013, 12:54 a.m., Mark Grover wrote: > > bigtop-test-framework/src/main/groovy/org/apache/bigtop/itest/shell/Shell.groovy, > > line 71 > > <https://reviews.apache.org/r/9166/diff/1/?file=253572#file253572line71> > > > > Hi Hari, this is when java/groovy starts to get hairy for me so I wrote > > a test in groovy. Here is how it looks: > > private void test(int x, Object... args){ > > System.out.println("with x") > > } > > > > private void test(Object... args){ > > System.out.println("without x") > > } > > test(1,2,3) > > test('a','b','c') > > test("abc") > > test(1) > > test(1, 'a') > > > > The output is: > > with x > > without x > > without x > > with x > > with x > > > > Conclusion: If the first argument of the call is int, the method with > > 1st int argument gets called (i.e. the signature you are adding) directly, > > bypassing the existing method that just takes Object... > > > > Do you see any concerns here? > > > > 1. Do you think those concerns may be resolved by making the newly > > signature private? > > 2. Even better, do you think there is a way to accomodate the > > additional timeout as a part of the existing arguments parameter without > > changing the method signature?
Makes sense. I see one way to solve this, by adding an enum which is used only for this one purpose. - Hari ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9166/#review15912 ----------------------------------------------------------- On Jan. 30, 2013, 8:35 p.m., Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9166/ > ----------------------------------------------------------- > > (Updated Jan. 30, 2013, 8:35 p.m.) > > > Review request for bigtop and Roman Shaposhnik. > > > Description > ------- > > Added new methods which can execute a script for a specified timeout and can > also execute in the background. > > > This addresses bug BIGTOP-835. > https://issues.apache.org/jira/browse/BIGTOP-835 > > > Diffs > ----- > > > bigtop-test-framework/src/main/groovy/org/apache/bigtop/itest/shell/Shell.groovy > ae3da68 > > bigtop-test-framework/src/test/groovy/org/apache/bigtop/itest/shell/ShellTest.groovy > 1571e10 > > Diff: https://reviews.apache.org/r/9166/diff/ > > > Testing > ------- > > Added unit tests for new functionality. Current unit tests pass > > > Thanks, > > Hari Shreedharan > >
