Skip to content
This repository has been archived by the owner on Sep 12, 2021. It is now read-only.

Give commands access to input stream. #70

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Give commands access to input stream. #70

wants to merge 1 commit into from

Conversation

guw
Copy link
Contributor

@guw guw commented Dec 6, 2016

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 guw mentioned this pull request Dec 6, 2016
@rherrmann
Copy link
Owner

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
Copy link
Contributor Author

guw commented Dec 6, 2016

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

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.

Signed-off-by: Gunnar Wagenknecht <gunnar@wagenknecht.org>
@rherrmann
Copy link
Owner

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
Copy link
Contributor Author

guw commented Dec 8, 2016

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
Copy link
Owner

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants