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.
