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

Add support for Windows Terminal #8

Merged
merged 2 commits into from
Aug 20, 2024

Conversation

dropsonic
Copy link
Contributor

Closes #7

@dropsonic
Copy link
Contributor Author

@jamestalmage could you please review&merge?

@jamestalmage
Copy link
Owner

jamestalmage commented Apr 6, 2021

@dropsonic I see the discussion here.

I understand that a properly implemented terminal will just ignore the special characters, and show the link text. But that can tend to clobber output formatting and alignments. The goal of this module isn't simply to avoid printing control characters (as mentioned, a properly implemented terminal won't display those characters anyways), but to allow authors to conditionally format their output based on how it will be displayed.

I think we maybe should do a version check. Is that possible? (If I'm understanding the conversations correctly, it seems no). I'm tempted to say get microsoft/terminal/issues/1040 sorted before we merge this. Otherwise we're going to end up clobbering a whole bunch of output.

@dropsonic
Copy link
Contributor Author

dropsonic commented Apr 6, 2021

@jamestalmage that's correct, there is no way to determine the actual version of Windows Terminal.
I understand your concerns, but in the meantime, the current version of the package also returns an invalid value for Windows Terminal, resulting in an improperly formatted output in the consumer apps.

So, basically, it is a choice between
(a) returning an invalid value for the up-to-date versions of Windows Terminal (as it is now)
or
(b) returning an invalid value for the outdated versions of Windows Terminal (as it is in this PR).

My assumption is that the vast majority of users install updates from Microsoft Store on time (even automatically) so, in my opinion, (b) is the preferred option.

@jamestalmage
Copy link
Owner

jamestalmage commented Apr 6, 2021

I'm not a Windows user, so I've got limited insights into how frequently Windows Terminal is updated.

I get your argument, and if Windows Terminal sees 99% auto-updates, then maybe it's a valid one. It would be good to understand that as well.

In the most common use case for this package, there's a big difference between returning a false negative, and a false positive. I think this use from eslint-formatter-pretty is fairly representative. If we were to give a false negative, you are inconvenienced and have to google the corresponding eslint rule to find the documentation. If we give a false positive, the output is clobbered and almost unusable.

So, I'm always going to be pretty conservative and favor false negatives over false positives. The bar has to be higher than just "It should be OK for most users", we need to ensure it is right for very nearly everybody. Before this package, tool authors were avoiding terminal hyperlinks entirely to maintain the widest possible compatibility. This package encourages more hyperlink use by guaranteeing dependents their output will be usable in the widest array of circumstances.

If somebody from Microsoft is willing to provide some insight into the install base of non-supporting vs supporting terminals, I am willing to reconsider. But it seems like it would be a lot more straightforward for them to just implement any of the recommended solutions in microsoft/terminal/issues/1040. This won't be the last time someone wants to interrogate the terminal version.

@dropsonic
Copy link
Contributor Author

@jamestalmage makes total sense, thanks for the detailed explanation. Ok, let's wait.

@LitoMore
Copy link
Contributor

LitoMore commented Sep 8, 2022

I get your argument, and if Windows Terminal sees 99% auto-updates, then maybe it's a valid one. It would be good to understand that as well.

FYI:

They are officially recommend installing the Windows Terminal from Microsoft Store: https://github.com/microsoft/terminal#microsoft-store-recommended.

They also provided some ways to install Windows Terminal with package managers and easy to update/upgrade: https://github.com/microsoft/terminal#other-install-methods.

@nickwesselman
Copy link

Can this be merged now without the need for a version check? Windows terminal has supported links for almost 4 years now. Similar libraries in other languages include 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

Successfully merging this pull request may close these issues.

Incorrect results for Windows Terminal
5 participants