> On July 25, 2014, 2:15 p.m., Kenneth Giusti wrote: > > /trunk/qpid/extras/qmf/src/py/qmf/console.py, line 381 > > <https://reviews.apache.org/r/23897/diff/1/?file=641418#file641418line381> > > > > is the indentation correct - looks like else: lines up with 'for' > > (could be this reviewboard web interface)
The else is indeed indented with the for. An else on a for loop means "If the loop did NOT terminate with a break, then execute the statements in the else." So in this case, if there were no more schema arguments with a default, we need to break out of the while loop to avoid an infinite loop. > On July 25, 2014, 2:15 p.m., Kenneth Giusti wrote: > > /trunk/qpid/extras/qmf/src/py/qmf/console.py, line 412 > > <https://reviews.apache.org/r/23897/diff/1/?file=641418#file641418line412> > > > > nit: Can this be "if arg.name in kwargs:' instead of iterating the > > kwargs list for each argument? Yes! Don't know what I was thinking. "if arg.name in kwargs:" is the correct way to do that. I'll update the patch. > On July 25, 2014, 2:15 p.m., Kenneth Giusti wrote: > > /trunk/qpid/extras/qmf/src/py/qmf/console.py, line 1512 > > <https://reviews.apache.org/r/23897/diff/1/?file=641418#file641418line1512> > > > > Ugh, not sure if this method is actually used anywhere, but it's also > > checking the arg count. Maybe we can remove it? Or... it could be > > rewritten to get the object first (using getObjects), then call the method > > on the object directly in order to pick up the fix. > > > > I'd go with removing it if we can. qpid-tool uses it to call methods. I'll update the patch. - Ernie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23897/#review48728 ----------------------------------------------------------- On July 24, 2014, 9:04 p.m., Ernie Allen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23897/ > ----------------------------------------------------------- > > (Updated July 24, 2014, 9:04 p.m.) > > > Review request for qpid, Justin Ross and Kenneth Giusti. > > > Repository: qpid > > > Description > ------- > > This patch attempts to handle the cases where the number of arguments > supplied to a method call don't match the number of arguments required by the > schema. > > When there are too many arguments supplied, the assumption is that the client > is aware of a new argument, but the broker isn't. This is currently the case > when qpid-route is dealing with an older broker and calls link.bridge(). The > console will look for a named argument that was supplied, but isn't in the > broker's schema. If one is found, it is removed. This requires that the new > argument be named when it is passed to console. Otherwise we would have to > assume the last argument supplied was the extra one, and that would be risky. > > When there are too few arguments supplied, the assumption is that client is > out of date and isn't aware a new argument that the schema requires. This is > currently the case when an old qpid-route is dealing with a new broker and > calls link.bridge(). The console will look for arguments in the schema that > have default values (starting at the end of the schema). If one is found, it > is added with the default value. This requires that all new arguments be > given a default value in the schema. > > > Diffs > ----- > > /trunk/qpid/extras/qmf/src/py/qmf/console.py 1613170 > /trunk/qpid/tools/src/py/qpid-route 1613170 > > Diff: https://reviews.apache.org/r/23897/diff/ > > > Testing > ------- > > schema: > <method name="test1"> > <arg name="a" dir="I" type="bool"/> > <arg name="b" dir="I" type="bool" default="True"/> > <arg name="c" dir="I" type="uint16"/> > <arg name="d" dir="I" type="uint32" default="2"/> > </method> > > client calls with console calls broker with > (False, False, 42, 1) a:False, b:False, c:42, d:1 > (False, 42) a:False, b:True, c:42, d:2 > (False, False, 42) a:False, b:False, c:42, d:2 > (False, 42, d=666) a:False, b:True, c:42, d:666 > (False, 42, b=False) a:False, b:False, c:42, d:2 > (False, 42, d=666, b=True) a:False, b:True, c:42, d:666 > (True, True, 42, 1, e="z") a:True, b:True, c:42, d:1 > (True, 42, e="z") a:True, b:True, c:42, d:2 > (False, 42, b=False, e="z") a:False, b:False, c:42, d:2 > > (False) Incorrect number of arguments: expected 4, > got 3 > (False, False, 42, 1, 0) Incorrect number of arguments: expected 4, > got 5 > (True, True, 42, 1, b=True) Incorrect number of arguments: expected 4, > got 5 > (True, e="z") Incorrect number of arguments: expected 4, > got 3 > > The following calls fail with "Failed: Exception - invalid conversion: > Cannot convert a (/home/eallen/22/qpid/cpp/src/qpid/types/Variant.cpp:132)" > (False, "a", d=666) a:False, b:True, c:"a", d:666 > (True, "a", 42) a:True, b:"a", c:42, d:2 > > As you can see from the last tests, I did not add any type checking to ensure > the supplied arguments match the schema before I make the actual call. > > > Thanks, > > Ernie Allen > >
