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 2686144103; Wed, 29 May 2024 17:57:32 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EF46140273; Wed, 29 May 2024 17:57:31 +0200 (CEST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id 416614026F for ; Wed, 29 May 2024 17:57:30 +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 E4923339; Wed, 29 May 2024 08:57:53 -0700 (PDT) Received: from [10.1.25.43] (FVFG51LCQ05N.cambridge.arm.com [10.1.25.43]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 143B43F792; Wed, 29 May 2024 08:57:28 -0700 (PDT) Message-ID: <9169ad26-4b3e-4585-af80-5840efe3493a@arm.com> Date: Wed, 29 May 2024 16:57:27 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 7/8] dts: rework interactive shells Content-Language: en-GB To: Jeremy Spewock Cc: dev@dpdk.org, =?UTF-8?Q?Juraj_Linke=C5=A1?= , Paul Szczepanek References: <20240326190422.577028-1-luca.vizzarro@arm.com> <20240509112057.1167947-1-luca.vizzarro@arm.com> <20240509112057.1167947-8-luca.vizzarro@arm.com> From: Luca Vizzarro In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 28/05/2024 22:07, Jeremy Spewock wrote: >> + if start_on_init: >> + self.start_application() > > What's the reason for including start_on_init? Is there a time when > someone would create an application but not want to start it when they > create it? It seems like it is always true currently and I'm not sure > we would want it to be delayed otherwise (except in cases like the > context manager patch where we want to enforce that it is only started > for specific periods of time). You are right in thinking that currently it's not used. I left it there on purpose as I see the potential case in which we want to manipulate the settings before starting the shell. Albeit, as you said it's not needed currently. I can omit it if we don't care for now. >> + def __post_init__(self): > > Is the name of this method meant to mimic that of the dataclasses? It > might also make sense to call it something like `_post_init()` as just > a regular private method, I'm not sure it matters either way. Ack. > Additionally, I think in other super classes which contained functions > that were optionally implemented by subclasses we omitted the `pass` > and just left the function stub empty other than the doc-string. > Either way this does the same thing, but it might be better to make > them consistent one way or the other. Ack. >> - def _start_application(self, get_privileged_command: Callable[[str], str] | None) -> None: >> + def start_application(self) -> None: >> >> + def _make_start_command(self) -> str: > > It might make sense to put this above the start_application method > since that is where it gets called. Ack. >> - testpmd_shell = self.sut_node.create_interactive_shell( >> - TestPmdShell, privileged=True >> - ) >> + testpmd_shell = TestPmdShell() > > Maybe adding a parameter to this instantiation in the example would > still be useful. So something like `TestPmdShell(self.sut_node)` > instead just because this cannot be instantiated without any > arguments. Yes! Well spotted, this was missed.