Re: [PATCH 5/6] dts: add statefulness to InteractiveShell

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

On Wed, Apr 10, 2024 at 1:27 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 10/04/2024 07:53, Juraj Linkeš wrote:
> > I have a general question. What are these changes for? Do you
> > anticipate us needing this in the future? Wouldn't it be better to add
> > it only when we need it?
>
> It's been sometime since we raised this task internally. This patch and
> the next one arise from some survey done on old DTS test cases.
> Unfortunately, I can't pinpoint.
>
> Specifically for this patch though, the timeout bit is useful in
> conjunction with the related change in the next. Instead of giving an
> optional timeout argument to all the commands where we may want to
> change it, aren't we better off with providing a facility to temporarily
> change this for the current scope?
>

This is a good question. If the scope is just one command, then no. If
it's more than one, then maybe yes. I don't know which is better.

We should also consider that this would introduce a difference in API
between the interactive and non-interactive sessions. Do we want to do
this there as well?

Also, maybe set_timeout should be a property or we could just make
_timeout public.
And is_privileged should just be privileged, as it's a property (which
shouldn't contain a verb; if it was a method it would be a good name).

> >
> > On Thu, Mar 28, 2024 at 5:48 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
> >>
> >> On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
> >> <snip>
> >>> diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> >>> index a2c7b30d9f..5d80061e8d 100644
> >>> --- a/dts/framework/remote_session/interactive_shell.py
> >>> +++ b/dts/framework/remote_session/interactive_shell.py
> >>> @@ -41,8 +41,10 @@ class InteractiveShell(ABC):
> >>>       _stdout: channel.ChannelFile
> >>>       _ssh_channel: Channel
> >>>       _logger: DTSLogger
> >>> +    __default_timeout: float
> >>
> >> Only single underscores are used for other private variables, probably
> >> better to keep that consistent with this one.
> >>
> >
> > I agree, I don't see a reason for the double underscore.
>
> Ack.
>
> >
> >>>       _timeout: float
> >>>       _app_args: Params | None
> >>> +    _is_privileged: bool = False
> >> <snip>
> >>> 2.34.1
> >>>
>