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

Allow to check a io.Writer instead of raw file descriptor #80

Open
cardil opened this issue Feb 28, 2023 · 8 comments · May be fixed by #81
Open

Allow to check a io.Writer instead of raw file descriptor #80

cardil opened this issue Feb 28, 2023 · 8 comments · May be fixed by #81

Comments

@cardil
Copy link

cardil commented Feb 28, 2023

The lib allows checking TTY versus a raw file descriptor. It would be convenient to allow checking of a given io.Writer instead.

Related mattn/go-colorable#65

@cardil cardil linked a pull request Feb 28, 2023 that will close this issue
@dolmen
Copy link
Contributor

dolmen commented Mar 15, 2023

Also: io.Reader.

@mattn
Copy link
Owner

mattn commented Mar 15, 2023

tty should always be checked with Writer.

@mattn
Copy link
Owner

mattn commented Mar 15, 2023

Why can't pass raw file?

@cardil
Copy link
Author

cardil commented Mar 15, 2023

Why can't pass raw file?

When you can pass a io.Writer, you can also pass any type that implements it, file as well.

@mattn
Copy link
Owner

mattn commented Mar 15, 2023

You can do type assertion before pass to the function like:

isatty.IsTerminal(w.(*os.File).Fd())

@dolmen
Copy link
Contributor

dolmen commented Mar 22, 2023

@mattn wrote:

tty should always be checked with Writer.

I disagree. I usually check if os.Stdin is a tty in programs that optionally accepts data on stdin. This allows to skip stdin or to show usage when the user didn't redirect stdin.

@dolmen
Copy link
Contributor

dolmen commented Mar 23, 2023

The time that I had a look at this package I also thought: why not an io.Writer?

But after that initial thought, I realized that the current API is very appropriate because:

  1. checking for tty and applying wrappers to handle is something that must be done early in the program, usually in the main package, where you have access to raw os handles. The current API works fine for that use case. High level libraries (such as loggers) must instead work on abstractions such as io.Writer and it should not be their responsability to do dirty stuff like applying wrappers to OS handles.
  2. the fact that you need to have access to the raw file descriptors to use the functions of go-isatty IS a feature as it blocks bad uses of go-isatty deeply in libraries. It forces users of the API to design the program in a way and that is a good thing because calls to go-isatty are costly because of syscalls.
  3. adding a wrapper to allow easy access to io.Writer/io.Reader would just make it easy to use go-isatty incorrectly (for example on every call to a log method of a logger object) and that would be just bad for the whole Go ecosystem.
  4. the proposed wrappers for io.Writer/io.Reader can easily be written outside of go-isatty. Making them available in the go-isatty module brings no benefits compared to making them available is a separate package, but brings the drawback of 3.

@GwynethLlewelyn
Copy link

Thanks for your comments. I had the same issue as @cardil and was going to ask how to implement this.

Using isatty.IsTerminal(w.(*os.File).Fd()) is insane 😁 but it totally works 😮

And that's more than enough for me. My use case is simple: I just wish to check if I'm writing logs to a TTY or to a file, and use coloured output if it's a TTY. Many packages already do that; some don't, and so I have to check if the output is a TTY before using some of the features — but as @dolmen commented, this happens very early in main() and is just requested once.

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

Successfully merging a pull request may close this issue.

4 participants