----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23897/#review48728 -----------------------------------------------------------
Ship it! Looks good, with a few minor nits below. I think we should take a page from Python and only allow default arguments to come after non-default positional arguments. I don't think our current schema parser enforces this. I wouldn't make that change as part of this fix - maybe open a JIRA to address that later. Thoughts? /trunk/qpid/extras/qmf/src/py/qmf/console.py <https://reviews.apache.org/r/23897/#comment85482> is the indentation correct - looks like else: lines up with 'for' (could be this reviewboard web interface) /trunk/qpid/extras/qmf/src/py/qmf/console.py <https://reviews.apache.org/r/23897/#comment85481> nit: Can this be "if arg.name in kwargs:' instead of iterating the kwargs list for each argument? /trunk/qpid/extras/qmf/src/py/qmf/console.py <https://reviews.apache.org/r/23897/#comment85480> 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. - Kenneth Giusti 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 > >
