Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use standard error #5

Open
notramo opened this issue Nov 11, 2022 · 9 comments
Open

Use standard error #5

notramo opened this issue Nov 11, 2022 · 9 comments

Comments

@notramo
Copy link

notramo commented Nov 11, 2022

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

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

notramo commented Nov 12, 2022

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

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

notramo commented Nov 13, 2022

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

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

notramo commented Feb 27, 2023

@omgtehlion, what about accepting a Writer?

@omgtehlion
Copy link
Contributor

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

@joachimschmidt557
Copy link
Owner

joachimschmidt557 commented Mar 9, 2023

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

notramo commented Mar 20, 2023

Could it detect buffered, and flush it?

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

No branches or pull requests

3 participants