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

feat: enable colors on Windows #2896

Merged
merged 4 commits into from
Oct 5, 2021
Merged

feat: enable colors on Windows #2896

merged 4 commits into from
Oct 5, 2021

Conversation

mattn
Copy link
Contributor

@mattn mattn commented Oct 5, 2021

On Windows, colors are not enabled. This change enable colors.
image
image

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

@mattn mattn requested a review from a team as a code owner October 5, 2021 14:30
@mattn mattn requested review from iamhopaul123 and removed request for a team October 5, 2021 14:30
@efekarakus efekarakus changed the title Enable colors on Windows feat: enable colors on Windows Oct 5, 2021
Copy link
Contributor

@efekarakus efekarakus left a comment

Choose a reason for hiding this comment

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

Hi @mattn ! Thank you so much for the contribution, your library is awesome 😍

I just have a question below please 🙇

@@ -25,16 +26,23 @@ func init() {
cobra.EnableCommandSorting = false // Maintain the order in which we add commands.
}

func main() {
func run() int {
defer colorable.EnableColorsStdout(nil)()
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why it wasn't working before 🤔
We're using the following writers when writing to stdout:

DiagnosticWriter = color.Error
OutputWriter = color.Output

which uses your library 🤩 as far as I understand:
https://github.com/fatih/color/blob/a05da93ebe62ca9fc6791d3376ec4dad01196448/color.go#L25-L30

Shouldn't that have been working already or am I misunderstanding how colorable.NewColorableStdout() works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, copilot-cli already use go-colorable. Okay, I'll fix in another way soon.

@efekarakus
Copy link
Contributor

Side note, is it possible to update the commit like this: feat: enable colors on Windows? so that it makes our CI checks happy 😆

@mattn
Copy link
Contributor Author

mattn commented Oct 5, 2021

I'll squash commits if you want.

@efekarakus
Copy link
Contributor

I'll squash commits if you want.

Not needed, thank you for the contribution!

Copy link
Contributor

@Lou1415926 Lou1415926 left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution and your awesome library 😻

@mergify mergify bot merged commit 03863ab into aws:mainline Oct 5, 2021
@mattn
Copy link
Contributor Author

mattn commented Oct 5, 2021

Thank you!

thrau pushed a commit to localstack/copilot-cli-local that referenced this pull request Dec 9, 2022
<!-- Provide summary of changes -->

On Windows, colors are not enabled. This change enable colors.
![image](https://user-images.githubusercontent.com/10111/136043356-dcf6534b-6d03-48a2-ae10-5e1f20cbd136.png)
![image](https://user-images.githubusercontent.com/10111/136043405-0f86fa79-bac8-4bb5-b968-26cf8e2bbcc0.png)


<!-- Issue number, if available. E.g. "Fixes aws#31", "Addresses aws#42, 77" -->

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.
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