On 07/26/2017 01:35 PM, Stefan Hajnoczi wrote: > On Tue, Jul 25, 2017 at 11:30:16AM -0400, Cleber Rosa wrote: >> On 07/25/2017 09:37 AM, Stefan Hajnoczi wrote: >>> On Mon, Jul 24, 2017 at 02:44:37PM +0200, Amador Pahim wrote: >>>> Signed-off-by: Amador Pahim <apa...@redhat.com> >>>> Reviewed-by: Fam Zheng <f...@redhat.com> >>>> --- >>>> scripts/qemu.py | 10 +++++----- >>>> tests/qemu-iotests/iotests.py | 18 +++++++++--------- >>>> 2 files changed, 14 insertions(+), 14 deletions(-) >>> >>> Please don't do this, it encourages code duplication. Now arbitrary >>> users can start accessing the public field directly instead of adding a >>> reusable interfaces like add_monitor_telnet(), add_fd(), etc. >>> >> >> Judging from tests/qemu-iotests/iotests.py:VM and your comment above, I >> assume you see value in simple wrappers such as: >> >> def add_device(self, opts): >> self._args.append('-device') >> self._args.append(opts) >> return self >> >> I honestly do not see any value here. I do see value in other wrappers, >> such as add_drive(), in which default values are there for convenience, >> and a drive count is kept. In the end, my point is that the there are >> cases when a wrapper is just an annoyance and provides nothing more than >> the illusion of a better design. > > I don't see much value in simple wrappers either besides method chaining > (more on that below). > > Getting back to this patch, I can only review patches in the context of > the current code base. I don't know future plans you may have. The > change may make sense together with other new changes that use a public > args field in a useful way - it just doesn't make sense the way it has > been presented in isolation. >
I think I described what the intention is: support tests that use instances directly. But yes, I think it can be better backed by real code making use of it. > About method chaining, the current code seems to be written with method > chaining in mind: > > https://en.wikipedia.org/wiki/Method_chaining#Python > > If all arguments are methods then everything can be chained: > > (vm.add_fd(fd.fileno(), 1, 'image0') > .add_drive('fd:image0', interface='none') > .add_device('virtio-blk-pci,drive=drive0')) > > So I guess there is a small value in having add_device(). That said, > tests don't take advantage of method chaining much. > Right, just about the same amount of value of a more generic "add_arg(option, arguments=[])". IMO not even worth its maintenance costs. -- Cleber Rosa [ Sr Software Engineer - Virtualization Team - Red Hat ] [ Avocado Test Framework - avocado-framework.github.io ] [ 7ABB 96EB 8B46 B94D 5E0F E9BB 657E 8D33 A5F2 09F3 ]
signature.asc
Description: OpenPGP digital signature