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

windows: enable virtual terminal processing, fixes #169 #186

Merged
merged 1 commit into from
Mar 1, 2023

Conversation

martinlindhe
Copy link
Contributor

This PR supersedes #173 because I accidentally deleted local fork.

It is the same PR except x/sys bumped to 0.5.0.

@fatih
Copy link
Owner

fatih commented Mar 1, 2023

Thank you @martinlindhe

)

func init() {
// Opt-in for ansi color support for current process.
Copy link
Contributor

@pellared pellared Mar 1, 2023

Choose a reason for hiding this comment

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

Can we use NO_COLOR env var and NoColor = true global variable to opt-out?
In such configuration this code should not be executed.

#173 (comment)

Copy link
Owner

Choose a reason for hiding this comment

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

the NO_COLOR env already changes the NoColor value. So we can just check for the NoColor variable and it'll cover everything.

Copy link
Contributor

@pellared pellared Mar 1, 2023

Choose a reason for hiding this comment

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

So I guess it is just a matter of adding

if NoColor {
  return
}

right? 😉

@fatih let me know if you want me to address my own comments

Copy link
Owner

@fatih fatih Mar 1, 2023

Choose a reason for hiding this comment

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

The problem though is, it wouldn't work as expected. Imagine this situation:

  1. You import color and enable NO_COLOR.
  2. The init() is called, it check's NoColor, if it's false it will not set the configuration
  3. After a while though, you set NoColor = false.
  4. It still doesn't work, because init() will not run anymore. So we need a separate enviornment variable. NoColor is working fine as it's now, but it wouldn't work for opt-out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding #2. init() is run after all var () blocks, so NoColor would be set before init() is called.

Comment on lines +14 to +17
if err := windows.GetConsoleMode(out, &outMode); err == nil {
outMode |= windows.ENABLE_PROCESSED_OUTPUT | windows.ENABLE_VIRTUAL_TERMINAL_PROCESSING
_ = windows.SetConsoleMode(out, outMode)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent Error Flow

Suggested change
if err := windows.GetConsoleMode(out, &outMode); err == nil {
outMode |= windows.ENABLE_PROCESSED_OUTPUT | windows.ENABLE_VIRTUAL_TERMINAL_PROCESSING
_ = windows.SetConsoleMode(out, outMode)
}
if err := windows.GetConsoleMode(out, &outMode); err != nil {
return
}
outMode |= windows.ENABLE_PROCESSED_OUTPUT | windows.ENABLE_VIRTUAL_TERMINAL_PROCESSING
_ = windows.SetConsoleMode(out, outMode)

reference: https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pellared I sent yet another PR for your last 2 comments. See #187

mend-for-github.aaakk.us.kg bot referenced this pull request in DelineaXPM/dsv-cli May 25, 2023
#110)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [github.com/fatih/color](https://togithub.com/fatih/color) | require |
minor | `v1.14.1` -> `v1.15.0` |

---

### ⚠ Dependency Lookup Warnings ⚠

Warnings were logged while processing this repo. Please check the
Dependency Dashboard for more information.

---

### Release Notes

<details>
<summary>fatih/color</summary>

### [`v1.15.0`](https://togithub.com/fatih/color/releases/tag/v1.15.0)

[Compare
Source](https://togithub.com/fatih/color/compare/v1.14.1...v1.15.0)

#### What's Changed

- windows: enable virtual terminal processing, fixes
[#&#8203;169](https://togithub.com/fatih/color/issues/169) by
[@&#8203;martinlindhe](https://togithub.com/martinlindhe) in
[https://github.com/fatih/color/pull/186](https://togithub.com/fatih/color/pull/186)
- ci: update dependencies by [@&#8203;fatih](https://togithub.com/fatih)
in
[https://github.com/fatih/color/pull/191](https://togithub.com/fatih/color/pull/191)
- Bump golang.org/x/sys from 0.5.0 to 0.6.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/fatih/color/pull/189](https://togithub.com/fatih/color/pull/189)
- Refactor color_windows.go by
[@&#8203;pellared](https://togithub.com/pellared) in
[https://github.com/fatih/color/pull/188](https://togithub.com/fatih/color/pull/188)

#### New Contributors

- [@&#8203;martinlindhe](https://togithub.com/martinlindhe) made their
first contribution in
[https://github.com/fatih/color/pull/186](https://togithub.com/fatih/color/pull/186)

**Full Changelog**:
fatih/color@v1.14.1...v1.15.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 3am on Monday" (UTC),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS41Ni4wIiwidXBkYXRlZEluVmVyIjoiMzUuNTYuMCJ9-->

Co-authored-by: mend-for-github.aaakk.us.kg[bot] <50673670+mend-for-github.aaakk.us.kg[bot]@users.noreply.github.com>
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.

3 participants