gonsole icon indicating copy to clipboard operation
gonsole copied to clipboard

Give commands access to input stream.

Open guw opened this issue 7 years ago • 5 comments

Some commands may read additional data from a user. In this case, they should have access to the input stream provided by the console for further processing.

@rherrmann This is a proposal for discussion. Let me know if you would like a different approach. I'll add tests once we settled onto an implementation. Also, let me know if you need an issue for this one for tracking purposes.

guw avatar Dec 06 '16 01:12 guw

Yes, an issue would be nice. I absolutely agree that for some commands it might be necessary to have access to the input stream.

However, I would prefer to use a higher abstraction on the API side that hides character encoding/decoding and ideally also end-of-input handling. Would it be possible to have something similar like the ConsoleOutput? For example

interface ConsoleInput {
  String readLine();
  // or, if necessary
  char read();
}

rherrmann avatar Dec 06 '16 10:12 rherrmann

@Yes, I was thinking about such an abstraction as I noticed similar one for output. However, they do add complexity. They are hiding just the console side of things. That hiding is also dangerous because users have still to implement it correctly at the command side of things.

Actually, I was about to start another discussion about lifting the abstraction for the output side as well. Our console implementation spawns native processes for commands. I'm using zt-exec library and this makes it super easy for connecting process inputs and outputs based on streams. Most libraries do that. Hiding it in the console puts additional burden on command implementors to re-implement stream to console pumping.

guw avatar Dec 06 '16 11:12 guw

It's been a while since I worked with the Gonsole code. The console abstraction is a purely by-product of the JGit command console and was driven by these needs. Its output is strings in most cases but due to JGit's pgm API it also needs to pass through raw output stream. Therefore the ConsoleOutput provides both, a write(String) and a write(ConsoleWriter) variant to cater for both.

For symmetry reasons, I would still prefer to have a ConsoleInput interface. If your first requirement is to process input streams, I am fine with having a read(InputStream) method, or something similar, only.

rherrmann avatar Dec 07 '16 15:12 rherrmann

I see. Let me describe the issue I see with the write(ConsoleWriter) and read(ConsoleReader) methods.

The API for zt-exec looks like this:

new ProcessExecutor().command("..", "...")
      .redirectOutputAlsoTo(outputStream)
      .redirectErrorAlsoTo(errorOutputStream)
      .redirectInput(inputStream)
      .execute();

Using the indirection/abstraction introduced by ConsoleWriter/ConsoleReader, the same code would look like this:

ProcessExecutor executor = new ProcessExecutor().command("..", "...");
stdOut.write(outputStream -> executor.redirectOutputAlsoTo(outputStream));
errorOut.write(outputStream -> executor.redirectErrorAlsoTo(outputStream));
stdIn.read(inputStream -> executor.redirectInput(inputStream));
executor.execute();

Thankfully, lambdas help in keeping the code concise. However, do you see the "abuse" issue? I have to use the write method not for writing to the output but for getting the OutputStream and passing it to the ProcessExecutor. Similar for read and InputStream. The references to those are extended beyond the actual ConsoleWriter/ConsoleReader lifespan. That just feels wrong.

Thoughts?

guw avatar Dec 08 '16 12:12 guw

Thank you for the elaborate reply, I see your point. Changing Gonsole to (additionally) provide plain streams seems a. not so small task and I am not sure if I will find the time to review such a change. Thus if you can write an adapter that wraps the executor and handles the ConsoleWriter/Reader indirection on your end of things, that seems less effort to me. If that wouldn't work for you and you are patient enough I will be happy to review change as I find the time.

rherrmann avatar Dec 11 '16 23:12 rherrmann