joachimschmidt557 / linenoize

A port of linenoise to zig
MIT License
55 stars 9 forks source link

Use standard error #5

Open notramo opened 1 year ago

notramo commented 1 year ago

I am using this library in a program that needs to do user interaction on stderr, in order to pipe useful data from stdout. Would it be possible to configure the file descriptor? Not only stdin/stdout, but arbitrary file descriptor, because some programs use stdout for data output, stderr for error messages, and /dev/tty for UI. If /dev/tty would be supported, I would probably switch my program to use that.

joachimschmidt557 commented 1 year ago

That's a great idea! Do you think amending the Linenoise.linenoise to include an optional parameter set (similar to std.heap.GeneralPurposeAllocator would be an acceptable solution? I'll also consider defaulting the file descriptors to /dev/tty instead of getStdIn() or getStdOut() (for platforms where that is supported).

notramo commented 1 year ago

I would rather add a file_descriptor field to the Linenoise struct, whic could be changed, similarly to e.g. masked mode. This way, calling the .linenoise() function would have less visual clutter. In my program, I would set it once, at start, and then use that file descriptor in all subsequent prompts in the whole program. Also, if subroutines are used in the program logic, it's enough to pass the Linenoise struct instance as argument, which has the file descriptor already set, instead of a Linenoise and a file descriptor.

var ln = Linenoise.init(allocator);

// read from stdin
ln.linenoise("This is read from stdin: ");

// read from tty
var tty = std.fs.openFileAbsolute("/dev/tty", .{ });
ln.file_descriptor = tty;
ln.linenoise("This is read using /dev/tty: ");

// read from stderr
ln.file_descriptor = std.io.getStdErr();
ln.linenoise("This is read using stderr: ");

If setting the file descriptor requires more complicated internal logic than assigning to a variable, then it could be ln.setFileDescriptor() instead of ln.file_descriptor =

joachimschmidt557 commented 1 year ago

Ok, putting it in the Linenoise struct is a good idea. How should the responsibility of closing the file descriptors be handled? If the pattern is to assign to the struct field, re-assigning that field without closing the existing file descriptor can be a common mistake. On the other hand, if a potential setFileDescriptor function always closed the existing file descriptor before assigning the new one, this could lead to errors if the previous file descriptor was e.g. stdin.

notramo commented 1 year ago

I would recommend not closing it in Linenoise. It could have unvanted effects. Simply leave it to the programmer to manage it as they want. I guess, most of the time, the file descriptor would not be closed: switching Linenoise from stdout to stderr, but still using stdout for other purposes, but this is the case also when switching to /dev/tty, but still using stdout/stderr for outputting custom data. In the few cases where closing is desirable, defer filedescriptor.close() would cover most of the cases.

omgtehlion commented 1 year ago

I have similar concerns not only about stdout/stderr, but more generally: it would be much better to pass and File struct as an input / output.

Especially for programs that can consume piped input, but still want to display text user interface on a /dev/tty.

I made a fork with added constructor: see https://github.com/omgtehlion/linenoize-win32/blob/b4ed601f86151b5ab0b12332a47107e504ab6ca0/src/main.zig#L178

Not still sure, do we need a full File struct or only Handle

notramo commented 1 year ago

@omgtehlion, what about accepting a Writer?

omgtehlion commented 1 year ago

Yeah, a pair of std.io.Writer and .Reader would be the ultimate solution. But this will require quite a few code changes.

joachimschmidt557 commented 1 year ago

I prefer the File approach. Although using a Reader and Writer would enable testing of linenoize, it imo creates a big footgun where users of linenoize may pass a buffered reader/writer to linenoize and linenoize does not flush the buffered writer.

notramo commented 1 year ago

Could it detect buffered, and flush it?