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

Consider replacing out.OutputIsRedirected by ctxutil.IsTerminal(ctx) #2025

Closed
AnomalRoil opened this issue Nov 8, 2021 · 1 comment · Fixed by #2248
Closed

Consider replacing out.OutputIsRedirected by ctxutil.IsTerminal(ctx) #2025

AnomalRoil opened this issue Nov 8, 2021 · 1 comment · Fixed by #2248
Assignees
Labels
cleanup Code hygiene
Milestone

Comments

@AnomalRoil
Copy link
Member

It seems we might have redundant handling of the "Are we in a terminal?" and "is output being redirected?"

ctxutil.IsTerminal relies on https://github.com/mattn/go-isatty in order to tell if we are in a terminal or not, which is great because it is very cross-platform, even supporting Plan9 and Windows.

I recently added in #2019 a method called OutputIsRedirected in our out package, which is meant to say if we are being redirected or not:

// OutputIsRedirected returns true if the current os.Stdout is a pipe instead of a terminal
func OutputIsRedirected() bool {
o, _ := os.Stdout.Stat()
return (o.Mode() & os.ModeCharDevice) != os.ModeCharDevice
}

It is not clear to me if this is equivalent to saying whether or not we are in a terminal, but it's pretty clear it probably isn't very cross-platform as is.

If "not being a tty" is including "being redirected", then we might want to change this new function to rely on go-isatty/ctxutil.IsTerminal(ctx) instead...

Originally posted by in #2024 (comment)

@AnomalRoil AnomalRoil added the cleanup Code hygiene label Nov 11, 2021
@AnomalRoil AnomalRoil added this to the 1.14.0 milestone Dec 21, 2021
@dominikschulz dominikschulz modified the milestones: 1.14.0, 1.x.x Mar 12, 2022
@AnomalRoil
Copy link
Member Author

I just confirmed using the following example code from isatty:

package main

import (
	"fmt"
	"github.com/mattn/go-isatty"
	"os"
)

func main() {
	if isatty.IsTerminal(os.Stdout.Fd()) {
		fmt.Println("Is Terminal")
	} else if isatty.IsCygwinTerminal(os.Stdout.Fd()) {
		fmt.Println("Is Cygwin/MSYS2 Terminal")
	} else {
		fmt.Println("Is Not Terminal")
	}
}

That both piping and redirecting output returns "not a terminal", we can therefore replace the non-cross platform code for out.OutputIsRedirected with the cross platform ctxutil.IsTerminal function.

Sending a PR shortly.

@AnomalRoil AnomalRoil self-assigned this May 31, 2022
AnomalRoil added a commit that referenced this issue May 31, 2022
RELEASE_NOTES=[CLEANUP] Deprecate OutputIsRedirected in favour of IsTerminal

Fixes #2025

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
dominikschulz pushed a commit that referenced this issue May 31, 2022
* Use IsTermninal instead of OutputIsRedirected

RELEASE_NOTES=[CLEANUP] Deprecate OutputIsRedirected in favour of IsTerminal

Fixes #2025

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>

* Fix lint

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
kpitt pushed a commit to kpitt/gopass that referenced this issue Jul 21, 2022
* Use IsTermninal instead of OutputIsRedirected

RELEASE_NOTES=[CLEANUP] Deprecate OutputIsRedirected in favour of IsTerminal

Fixes gopasspw#2025

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>

* Fix lint

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code hygiene
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants