[PATCH v2 7/8] dts: rework interactive shells

[Luca Vizzarro]

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:
>> <snip>
>> +    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.