From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id D6BC9431F0; Tue, 24 Oct 2023 14:21:51 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C648040ED3; Tue, 24 Oct 2023 14:21:51 +0200 (CEST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id 6FCCE40ED3 for ; Tue, 24 Oct 2023 14:21:50 +0200 (CEST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id C07CE2F4; Tue, 24 Oct 2023 05:22:30 -0700 (PDT) Received: from [10.1.38.181] (e125442.arm.com [10.1.38.181]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 14D443F64C; Tue, 24 Oct 2023 05:21:47 -0700 (PDT) Message-ID: Date: Tue, 24 Oct 2023 13:21:46 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC PATCH v4 1/4] dts: code adjustments for sphinx Content-Language: en-US To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, bruce.richardson@intel.com, jspewock@iol.unh.edu, probb@iol.unh.edu, dev@dpdk.org References: <20230511091408.236638-1-juraj.linkes@pantheon.tech> <20230831100407.59865-1-juraj.linkes@pantheon.tech> <20230831100407.59865-2-juraj.linkes@pantheon.tech> <69c39f25-563e-4e32-a6a2-81e0049332d5@foss.arm.com> From: Yoan Picchi In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 10/24/23 07:39, Juraj Linkeš wrote: > On Mon, Oct 23, 2023 at 1:52 PM Yoan Picchi wrote: >> >> On 10/23/23 07:44, Juraj Linkeš wrote: >>> >>> >>>> >>>> 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.