On 10/24/23 07:39, Juraj Linkeš wrote:
On Mon, Oct 23, 2023 at 1:52 PM Yoan Picchi <yoan.pic...@foss.arm.com> wrote:
On 10/23/23 07:44, Juraj Linkeš wrote:
<snip>
My only nitpick comment would be on the name of the file common.py that
only contain the MesonArgs class. Looks good otherwise
Could you elaborate a bit more, Yoan? The common.py module is supposed
to be extended with code common to all other modules in the
testbed_model package. Right now we only have MesonArgs which fits in
common.py, but we could also move something else into common.py. We
also could rename common.py to something else, but then the above
purpose would not be clear.
I'm finishing the docstrings soon so expect a new version where things
like these will be clearer. :-)
My issue with the name is that it isn't clear what is the purpose of
this file. It only tell to some extend how it is used.
Well, the name suggests it's code that's common to other modules, as
in code that other modules use, just like the MesonArgs class, which
is used in three different modules. I've chosen common.py as that's
what some of the DPDK libs (such as EAL) seem to be using for this
purpose. Maybe there's a better name though or we could move the class
elsewhere.
If we start adding more things in this file, then I see two options:
- Either this is related to the current class, and thus the file could
be named meson_arg or something along those lines.
- Or it is unrelated to the current class, and we end up with a file
coalescing random bits of code, which is usually a bit dirty in OOP.
It's code that's reused in multiple places, I'm not sure whether that
qualifies as random bits of code. It could be in os_session.py (as
that would work import-wise), but that's not a good place to put it,
as the logic is actually utilized in sut_node.py. But putting it into
sut_node.py doesn't work because of imports. Maybe we could just put
it into utils.py in the framework dir, which is a very similar file,
if not the same. My original thoughts were to have a file with common
code in each package (if needed), depending on where the code is used
(package level-wise), but it looks like we can just have this sort of
common utilities on the top level.
Like I said, it's a bit of a nitpick, but given it's an RFC I hope
you'll give it a thought in the next version.
I thought a lot about this before submitting this RFC, but I wanted
someone to have a look at this exact thing - whether the common.py
file makes sense and what is the better name, common.py or utils.py
(which is why I have both in this patch). I'll move the MesonArgs
class to the top level utils.py and remove the common.py file as that
makes the most sense to me now.
If you have any recommendations we may be able to make this even better.
I didn't meant to imply you did not think a lot about it, sorry if it
came that way.
I prefer the idea of utils.py to a common.py, be it at package level or
above. There might also be the option of __init__.py but I'm not sure
about it.
That being said, I'm relatively new to dpdk and didn't know common.py
was a common thing in EAL so I'll leave it up to you.
