On 07/25/2017 09:37 AM, Stefan Hajnoczi wrote: > On Mon, Jul 24, 2017 at 02:44:37PM +0200, Amador Pahim wrote: >> Let's make args public so users can extend it without feeling like >> abusing the internal API. > > Nothing is abusing an internal API. PEP8 describes the difference > between public (no underscore), protected aka subclass API (single > underscore), and private fields (single or double underscore): > > https://www.python.org/dev/peps/pep-0008/ > > _args is a protected field. It is perfectly normal for a subclass to > access a protected field in its parent class. >
Right, which means that instances should not be using it. I guess
there's a clear conflict of what is assumed to be the intended use for
this class.
I can see the value in using it *directly*, and I can also see changing
the QEMU args as a requirement.
>> 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.
If "args" is not made public, wrappers with no real value will start to
be proposed, either within the test's own subclass (like in
tests/qemu-iotests/iotests.py:VM) or will make its way to
scripts/qemu.py:QEMUMachine.
What you describe as "adding reusable interfaces" can be seen both as a
blessing if they're really well designed interfaces, or a curse if
they're something a test writer *has* to write while a test is being
written to get around the fact that "args" is not available.
Extending on the "add_device()" example, how would you feel about
something like:
def add_arg(self, arg, opts=None):
self._args.append(arg)
if opts is not None:
self._args.append(opts)
FIY, I have a bad feeling about it, but it's quite comparable to
add_device(), and it keeps "args" private.
--
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
