Reduce verbosity of getting a session with a logged stream

[dsherret]

Just testing out this crate and it works great so far!

What I've found though, is sometimes I want to see the output of what's happening and so I use a session with a logged stream. The types I had to write are extremely verbose though:

#[cfg(unix)]
type PtyProcess = expectrl::process::unix::UnixProcess;
#[cfg(windows)]
type PtyProcess = expectrl::process::windows::WinProcess;

#[cfg(unix)]
type PtyProcessStream = expectrl::process::unix::PtyStream;
#[cfg(windows)]
type PtyProcessStream = expectrl::process::windows::ProcessStream;

type PtyLoggedStream =
  expectrl::stream::log::LoggedStream<PtyProcessStream, io::Stderr>;
type PtyLoggingSession = expectrl::Session<PtyProcess, PtyLoggedStream>;

I noticed some of these are defined in the crate, but they're not public. Perhaps they should be or perhaps there should be some easier way of doing this? I'm using this crate for testing, so I'm creating my own wrapper API on top of this where I can just set an option to get logging on, but it's quite verbose so far.

[zhiburt]

Hi @dsherret

Thank you for opening it up.

The types I had to write are extremely verbose though:

Yes.....
It something I was wondering about a number of times.

Perhaps they should be or perhaps there should be some easier way of doing this?

You're talking only on behalf of logged session right?
Cause the Session type has default parameters and it should not be an issue, I believe.

According to logged stream.
We could create an alias as an example above; or we could create a new type for it; the type may be handy if we willing to add more functions to it, see.

// variant 1
session::log(original_session, output);
// variant 2
LogSession::stdout(original_session);
LogSession::stderr(original_session);;
LogSession::new(original_session, stream);

struct LogSession<W, S = Session> {}

Take care

[dsherret]

In this case, I'm returning the value from a function, so when I switch it from a regular stream to a logged stream I don't want to have a separate compilation or need to manually change the type. What I've done is created a trait for a session, then put that in a box.

Here's what my code looks like at the moment:

#[cfg(unix)]
type PtyProcess = expectrl::process::unix::UnixProcess;
#[cfg(windows)]
type PtyProcess = expectrl::process::windows::WinProcess;
#[cfg(unix)]
type PtyProcessStream = expectrl::process::unix::PtyStream;
#[cfg(windows)]
type PtyProcessStream = expectrl::process::windows::ProcessStream;
type PtyLoggedStream =
  expectrl::stream::log::LoggedStream<PtyProcessStream, io::Stderr>;
type PtyLoggingSession = expectrl::Session<PtyProcess, PtyLoggedStream>;

trait ExpectrlSession: Read + Write {
  fn send_line(&mut self, s: &str) -> std::io::Result<()>;
  fn expect(&mut self, s: &str) -> Result<expectrl::Captures, expectrl::Error>;
  fn try_read(&mut self, buf: &mut [u8]) -> std::io::Result<usize>;
}

macro_rules! implement_expectrl_session {
  ($struct_name:ident) => {
    impl ExpectrlSession for $struct_name {
      fn send_line(&mut self, s: &str) -> std::io::Result<()> {
        self.0.send_line(s)
      }

      fn expect(
        &mut self,
        s: &str,
      ) -> Result<expectrl::Captures, expectrl::Error> {
        self.0.expect(s)
      }

      fn try_read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> {
        self.0.try_read(buf)
      }
    }

    impl Read for $struct_name {
      fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
        self.0.read(buf)
      }
    }

    impl Write for $struct_name {
      fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
        self.0.write(buf)
      }

      fn flush(&mut self) -> io::Result<()> {
        self.0.flush()
      }
    }
  };
}

struct LoggingExpectrlSession(PtyLoggingSession);
implement_expectrl_session!(LoggingExpectrlSession);

struct DefaultExpectrlSession(expectrl::session::Session);
implement_expectrl_session!(DefaultExpectrlSession);

Then now I can have a common return value of Box<dyn ExpectrlSession>.

I almost wonder if it would be useful to have a trait implementation built into the library for a session. Then consumers could box any type that implements that trait.

Anyway, this is not a priority for me because I have this workaround that works ok. I'm just raising a pain point I found that other people might run into in the future as well.

[zhiburt]

I almost wonder if it would be useful to have a trait implementation built into the library for a session.

So I was pondering about it too, but decided not to.
I can't argue why to do so but I can't be against it either.

As I remember it was tried.
But the approach failed; specifically async_trait implementation caused some difficulties.

Plus as I remember I did not found a good name for a trait.
As I think it's better to separate expect_*/check_* functions from send_*.

Need to think about it again.

I'm returning the value from a function, so when I switch it from a regular stream to a logged stream I don't want to have a separate compilation or need to manually change the type. What I've done is created a trait for a session, then put that in a box.

Just a note that enum could work here too.

[zhiburt]

use std::io::{self, Stderr};

use expectrl::{
    session::{OsProcess, OsProcessStream},
    stream::log::LogStream,
    Captures, Session,
};

type LogSession = Session<OsProcess, LogStream<OsProcessStream, Stderr>>;

pub enum PtySession {
    Default(Session),
    Logged(LogSession),
}

impl PtySession {
    pub fn send_line(&mut self, text: &str) -> io::Result<()> {
        match self {
            PtySession::Default(s) => s.send_line(text),
            PtySession::Logged(s) => s.send_line(text),
        }
    }

    pub fn expect(&mut self, text: &str) -> Result<Captures, expectrl::Error> {
        match self {
            PtySession::Default(s) => s.expect(text),
            PtySession::Logged(s) => s.expect(text),
        }
    }

    pub fn try_read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
        match self {
            PtySession::Default(s) => s.try_read(buf),
            PtySession::Logged(s) => s.try_read(buf),
        }
    }
}

So I've exposed the aliases.

[tmpfs]

Hi,

So I am running into the same issue here, I want at runtime to either have logging enabled or not.

The enum solution works up to a point, it breaks down when we want to pass a session to ReplSession::new.

I really think this should be trait based. Are you willing to take a PR that fixes this?

Thanks for the library, it's got a much better API than rexpect.

[tmpfs]

One easier way to do this (avoiding adding a trait) would be to formalize support for the enum version by adding it to this library, currently it looks like this:

type LogSession = Session<UnixProcess, LogStream<PtyStream, Stdout>>;

pub enum PtySession {
    Default(Session),
    Logged(LogSession),
}

impl PtySession {
    pub fn send<B: AsRef<[u8]>>(&mut self, buf: B) -> io::Result<()> {
        match self {
            PtySession::Default(s) => s.send(buf),
            PtySession::Logged(s) => s.send(buf),
        }
    }

    pub fn send_line(&mut self, text: &str) -> io::Result<()> {
        match self {
            PtySession::Default(s) => s.send_line(text),
            PtySession::Logged(s) => s.send_line(text),
        }
    }

    pub fn expect<N>(
        &mut self,
        needle: N,
    ) -> std::result::Result<Captures, expectrl::Error>
    where
        N: Needle,
    {
        match self {
            PtySession::Default(s) => s.expect(needle),
            PtySession::Logged(s) => s.expect(needle),
        }
    }
}

impl Write for PtySession {
    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
        match self {
            PtySession::Default(s) => s.write(buf),
            PtySession::Logged(s) => s.write(buf),
        }
    }

    fn flush(&mut self) -> io::Result<()> {
        match self {
            PtySession::Default(s) => s.flush(),
            PtySession::Logged(s) => s.flush(),
        }
    }
}

impl BufRead for PtySession {
    fn fill_buf(&mut self) -> io::Result<&[u8]> {
        match self {
            PtySession::Default(s) => s.fill_buf(),
            PtySession::Logged(s) => s.fill_buf(),
        }
    }

    fn consume(&mut self, amt: usize) {
        match self {
            PtySession::Default(s) => s.consume(amt),
            PtySession::Logged(s) => s.consume(amt),
        }
    }
}

impl Read for PtySession {
    fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
        match self {
            PtySession::Default(s) => s.read(buf),
            PtySession::Logged(s) => s.read(buf),
        }
    }
}

Then we could just update the signature for ReplSession::new to take PtySession instead of Session for people that don't wany optional logging they can just wrap Session in PtySession::Default.

How does that sound?

[tmpfs]

It would be breaking change so would necessitate a bump to v0.8 I think.

[tmpfs]

@zhiburt I have had a go at sketching this out in this PR.

I think the trait makes documentation easier and exposes a consistent API for callers regardless of which type of session they are using.

[zhiburt]

Originally

It was supposed to be a new 'Stream' rather then a new 'Session'.

something like the following:

    let process = UnixProcess::spawn("bash").unwrap();
    let stream = process.get_pty_stream().unwrap();
    let stream = LogStream::new(stream, std::io::stdout());
    let session = Session::new(process, stream).unwrap();
    let mut repl = ReplSession::new(session, String::from("$"), None, false);

    repl.send_line("echo 1231234").unwrap();
    repl.send_line("echo 3242342").unwrap();

    repl.send_line("ls").unwrap();

    repl.read_line(&mut String::new()).unwrap();

Please share your thoughts