rherrmann / gonsole

Gonsole - Git Console for the Eclipse IDE
http://gonsole.codeaffine.com
38 stars 10 forks source link

Give commands access to input stream. #70

Open guw opened 7 years ago

guw commented 7 years ago

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.

rherrmann commented 7 years ago

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();
}
guw commented 7 years ago

@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.

rherrmann commented 7 years ago

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.

guw commented 7 years ago

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?

rherrmann commented 7 years ago

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.