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

fixes #58 detect non interactive terminals #83

Conversation

marcellourbani
Copy link
Contributor

No description provided.

@chshersh chshersh added terminal hacktoberfest-accepted https://hacktoberfest.com/participation/ labels Oct 6, 2022
Copy link
Owner

@chshersh chshersh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks great! But tests are needed.

src/Iris/Cli/Interactive.hs Show resolved Hide resolved
src/Iris/Cli/Interactive.hs Show resolved Hide resolved
src/Iris/Cli/Interactive.hs Outdated Show resolved Hide resolved
@marcellourbani
Copy link
Contributor Author

I'm having second thoughts on this implementation.

  • supportsANSI will fail on a dumb terminal, which can still be interactive, although without the bells and whistles
  • hIsTerminalDevice from System.IO would be better on non-windows, but will fail on mintty
  • isatty from hsshellscript looks Posix only, wonder wether it works in windows

So I plan to check both supportsANSI and hIsTerminalDevice, and only force noninteractive if both fail

PS: should we add an option to force interactivity to support tools like expect?

@marcellourbani marcellourbani force-pushed the fixes-#58-detect-non-interactive-terminals-automatically branch from e3daed8 to c668d55 Compare October 8, 2022 11:06
Copy link
Owner

@chshersh chshersh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few more comments to the implementation 🙂

Maybe, we can implement our own function that checks different terminal emulators? If it's Posix, check the value of $-, if it's minTTY, check something else, and do something else for PowerShell or cygwin.

Maybe we can also check what ghcup does and how they handle cross-platform support?

test/Test/Iris/Cli.hs Outdated Show resolved Hide resolved
src/Iris/Cli/Interactive.hs Outdated Show resolved Hide resolved
src/Iris/Cli/Interactive.hs Outdated Show resolved Hide resolved
@chshersh
Copy link
Owner

chshersh commented Oct 9, 2022

@marcellourbani The fixes look great 👍🏻
I believe, the only difficulties left are:

  • Proper cross-platform interactivity detection
  • Separate handling for stdout and stderr

I'm not an expert on the first one. So I don't have a strong opinion on this. We can go with the simplest reasonable option for now and change later when it's not enough. Nobody pays me to support exotic shells. So if the standard library doesn't have an out-of-the-box cross-platform solution, what can I do 🤷🏻

I agree that the --no-input flag is responsible for multiple use cases:

  1. Disabling asking questions.
  2. Don't do interactive things like progress bars.

As for the second one, let's check only stdin. I've consulted the CLI guidelines and they recommend checking only stdin:

I believe, I confused --no-input with --no-colour a bit 🥴

So, a simple solution would be to just run hSupportsANSI on stdout and be done with it 👏🏻

@marcellourbani
Copy link
Contributor Author

marcellourbani commented Oct 10, 2022

@chshersh

Nobody pays me to support exotic shells. So if the standard library doesn't have an out-of-the-box cross-platform solution, what can I do 🤷🏻

What about letting the user override?

data RequestedInteractiveMode
    = RInteractive
    | RNonInteractive
    | RAutoDetect

data InteractiveMode
    = Interactive
    | NonInteractive

handleInteractiveMode :: RequestedInteractiveMode -> IO InteractiveMode

or simply:

handleInteractiveMode :: Maybe RequestedInteractiveMode -> IO InteractiveMode

so by default we autodetect, but users can force either way

@chshersh
Copy link
Owner

@marcellourbani Overriding interactivity is something I'd like to avoid. We can't output interactive info in a non-interactive terminal so it doesn't make sense. Also, there's no appropriate CLI flag for that and since it's a global default, I'd like to avoid polluting global namespace with non-standard flags.

I'm happy to accept support of exotic shells like minTTY and Cygwin. So, if you want to work on supporting and testing them, feel free to continue working on this issue and taking as much time as you want 🤗

I was mostly implying that the changes might be unreliable without the ability to test this on CI, so the support will be potentially fragile. And if someone wants to contribute the support of these shells and verify that it works for them, I'm happy to accept this change! But I personally don't have the capacity for that. And I wouldn't ask contributors to do such huge work either 🙏🏻

Copy link
Owner

@chshersh chshersh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the implementation and so insightful conversation! 🙏🏻

@chshersh chshersh merged commit 4701eda into chshersh:main Oct 11, 2022
@marcellourbani
Copy link
Contributor Author

Thank you for the mentoring!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted https://hacktoberfest.com/participation/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants