On 11/04/2024 11:30, Juraj Linkeš wrote:
I've been thinking about these interactive shell constructors for some
time and I think the factory pattern is not well suitable for this.
Factories work well with classes with the same API (i.e.
implementations of abstract classes that don't add anything extra),
but are much less useful when dealing with classes with different
behaviors, such as the interactive shells. We see this here, different
apps are going to require different args and that alone kinda breaks
the factory pattern. I think we'll need to either ditch these
factories and instead just have methods that return the proper shell
(and the methods would only exist in classes where they belong, e.g.
testpmd only makes sense on an SUT). Or we could overload each factory
(the support has only been added in 3.11 with @typing.overload, but is
also available in typing_extensions, so we would be able to use it
with the extra dependency) where different signatures would return
different objects. In both cases the caller won't have to import the
class and the method signature is going to be clearer.
We have this pattern with sut/tg nodes. I decided to move away from
the node factory because it didn't add much and in fact the code was
only clunkier. The interactive shell is not quite the same, as the
shells are not standalone in the same way the nodes are (the shells
are tied to nodes). Let me know what you think about all this - both
Luca and Jeremy.
When writing this series, I went down the path of creating a
`create_testpmd_shell` method at some point as a solution to these
problems. Realising after that it may be too big of a change, and
possibly best left to a discussion exactly like this one.
Generics used at this level may be a bit too much, especially for
Python, as support is not *that* great. I am of the opinion that having
a dedicated wrapper is easier for the developer and the user. Generics
are not needed to this level anyways, as we have a limited selection of
shells that are actually going to be used.
We can also swap the wrapping process to simplify things, instead of:
shell = self.sut_node.create_interactive_shell(TestPmdShell, ..)
do:
shell = TestPmdShell(self.sut_node, ..)
Let the Shell class ingest the node, and not the other way round.
The current approach appears to me to be top-down instead of bottom-up.
We take the most abstracted part and we work our way down. But all we
want is concreteness to the end user (developer).
Let me illustrate this on the TestPmdShell __init__() method I had in mind:
def __init__(self, interactive_session: SSHClient,
logger: DTSLogger,
get_privileged_command: Callable[[str], str] | None,
app_args: EalTestPmdParams | None = None,
timeout: float = SETTINGS.timeout,
) -> None:
super().__init__(interactive_session, logger, get_privileged_command)
self.state = TestPmdState()
Where EalTestPmdParams would be something that enforces that
app_args.app_params is of the TestPmdParameters type.
But thinking more about this, we're probably better off switching the
params composition. Instead of TestPmdParameters being part of
EalParameters, we do it the other way around. This way the type of
app_args could just be TestPmdParameters and the types should work.
Or we pass the args separately, but that would likely require ditching
the factories and replacing them with methods (or overloading them).
And hopefully the imports won't be impossible to solve. :-)
It is what I feared, and I think it may become even more convoluted. As
you said, ditching the factories will simplify things and make it more
straightforward. So, we wouldn't find ourselves in problems like these.
I don't have a strong preference in approach between:
* overloading node methods
* dedicated node methods
* let the shells ingest nodes instead
But if I were to give priority, I'd take it from last to first. Letting
shells ingest nodes will decouple the situation adding an extra step of
simplification. I may not see the full picture though. The two are
reasonable but, having a dedicated node method will stop the requirement
to import the shell we need, and it's pretty much equivalent... but
overloading also is very new to Python, so I may prefer to stick to more
established.
Letting TestPmdParams take EalParams, instead of the other way around,
would naturally follow the bottom-up approach too. Allowing Params to
arbitrarily append string arguments – as proposed, would also allow
users to use a plain (EalParams + string). So sounds like a good
approach overall.
