-
Notifications
You must be signed in to change notification settings - Fork 32
feat: Adds ANSI color support for Windows terminals #39
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
base: main
Are you sure you want to change the base?
Changes from all commits
e52d8fd
22e2634
b22f972
345aae0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| //go:build windows | ||
|
|
||
| package godump | ||
|
|
||
| import ( | ||
| "log" | ||
|
|
||
| "github.com/goforj/godump/internal/ansi" | ||
| ) | ||
|
|
||
| // init activates ANSI support on Windows terminals by calling the Enable | ||
| // function from the internal ansi package. | ||
| // If enabling ANSI fails (e.g., not running in a real console), it logs | ||
| // the error but continues execution, as colors are optional. | ||
| func init() { | ||
| if err := ansi.Enable(); err != nil { | ||
| log.Printf("godump: failed to enable ANSI (likely due to output redirection): %v\n", err) | ||
| } | ||
| } |
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't add integration test. The lib you import are tested. The only thing you need I think is to test you the behavior of the code when you overload the result of IsTerminal There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly. We're on the same page. I just got the impression you thought I was still using a mock for that. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| package godump | ||
|
|
||
| import ( | ||
| "os" | ||
| "os/exec" | ||
| "strings" | ||
| "testing" | ||
| ) | ||
|
|
||
| // TestAnsiInNonTty verifies that no ANSI codes are produced when output is redirected. | ||
| func TestAnsiInNonTty(t *testing.T) { | ||
| // ANSI escape character. We expect this to be ABSENT from the output. | ||
| const escape = "\x1b" | ||
|
|
||
| // The source code for the program we're going to run. | ||
| const sourceCode = ` | ||
| package main | ||
| import "github.com/goforj/godump" | ||
| func main() { | ||
| s := struct{ Name string }{"test"} | ||
| godump.Dump(s) | ||
| } | ||
| ` | ||
|
|
||
| // Create a temporary directory to avoid package main collision. | ||
| tempDir := t.TempDir() | ||
| tempFile, err := os.CreateTemp(tempDir, "test_*.go") | ||
| if err != nil { | ||
| t.Fatalf("failed to create temp file: %v", err) | ||
| } | ||
|
|
||
| _, err = tempFile.WriteString(sourceCode) | ||
| if err != nil { | ||
| t.Fatalf("failed to write temp file: %v", err) | ||
| } | ||
| tempFile.Close() | ||
|
|
||
| // Run the program using `go run`. By capturing the output, we ensure | ||
| // that the program's stdout is not a TTY. | ||
| //nolint:gosec // tempFile.Name() is a controlled temporary file created by this test | ||
| cmd := exec.Command("go", "run", tempFile.Name()) | ||
| output, err := cmd.CombinedOutput() | ||
| if err != nil { | ||
| t.Fatalf("failed to run test program: %v\nOutput:\n%s", err, string(output)) | ||
| } | ||
|
|
||
| if strings.Contains(string(output), escape) { | ||
| t.Errorf("expected output to NOT contain ANSI escape codes when not in a TTY, but it did. Output:\n%s", string(output)) | ||
| } | ||
| } | ||
|
|
||
| // TestAnsiInTty verifies that ANSI codes are produced when FORCE_COLOR is set. | ||
| func TestAnsiInTty(t *testing.T) { | ||
| // ANSI escape character. We expect this to be PRESENT in the output. | ||
| const escape = "\x1b" | ||
|
|
||
| // The source code for the program we're going to run. | ||
| const sourceCode = ` | ||
| package main | ||
| import "github.com/goforj/godump" | ||
| func main() { | ||
| s := struct{ Name string }{"test"} | ||
| godump.Dump(s) | ||
| } | ||
| ` | ||
| // Create a temporary directory to avoid package main collision. | ||
| tempDir := t.TempDir() | ||
| tempFile, err := os.CreateTemp(tempDir, "test_*.go") | ||
| if err != nil { | ||
| t.Fatalf("failed to create temp file: %v", err) | ||
| } | ||
|
|
||
| _, err = tempFile.WriteString(sourceCode) | ||
| if err != nil { | ||
| t.Fatalf("failed to write temp file: %v", err) | ||
| } | ||
| tempFile.Close() | ||
|
|
||
| // Run the program using `go run`. By capturing the output, we ensure | ||
| // that the program's stdout is not a TTY. | ||
| //nolint:gosec // tempFile.Name() is a controlled temporary file created by this test | ||
| cmd := exec.Command("go", "run", tempFile.Name()) | ||
|
|
||
| cmd.Env = append(os.Environ(), "FORCE_COLOR=1") | ||
| output, err := cmd.CombinedOutput() | ||
| if err != nil { | ||
| t.Fatalf("failed to run test program: %v\nOutput:\n%s", err, string(output)) | ||
| } | ||
|
|
||
| if !strings.Contains(string(output), escape) { | ||
| t.Errorf("expected output to contain ANSI escape codes when FORCE_COLOR is set, but it didn't. Output:\n%s", string(output)) | ||
| } | ||
| } |
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't get the purpose of this. It's a package that does things, OK. The Enable is called via an init when using windows, OK. But then? I mean how does that interact or has effect of the fact godump calls term.IsTerminal ? I feel like the windowansi package and its initialization leads to something that is not used. What am I missing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So while they handle different concerns, they work together: one enables support, the other controls when it's appropriate to use it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel they should in the same module then I mean the init call to windowansi.Enable made for window module could be in the terminal module |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| //go:build windows | ||
|
|
||
| package ansi | ||
|
|
||
| import ( | ||
| "os" | ||
| "syscall" | ||
| "unsafe" | ||
| ) | ||
|
|
||
| const ( | ||
| SYS_CALL_FAILURE = 0 | ||
| enableVirtualTerminalProcessing = 0x0004 | ||
| ) | ||
|
|
||
| // Enable activates ANSI support on Windows terminals by setting the | ||
| // ENABLE_VIRTUAL_TERMINAL_PROCESSING flag. | ||
| // Returns an error if the output is not a console or if setting the mode fails. | ||
| func Enable() error { | ||
|
|
||
| // Load kernel32.dll and the necessary procedures dynamically. | ||
| // This avoids a hard dependency and allows the program to run on non-Windows | ||
| // systems, although this file is guarded by a build tag. | ||
| kernel32 := syscall.NewLazyDLL("kernel32.dll") | ||
| procGetConsoleMode := kernel32.NewProc("GetConsoleMode") | ||
| procSetConsoleMode := kernel32.NewProc("SetConsoleMode") | ||
|
|
||
| // Get the handle for standard output. | ||
| handle := syscall.Handle(os.Stdout.Fd()) | ||
| var mode uint32 | ||
|
|
||
| // GetConsoleMode fails if not in a real console. | ||
| ret, _, err := procGetConsoleMode.Call(uintptr(handle), uintptr(unsafe.Pointer(&mode))) | ||
| if ret == SYS_CALL_FAILURE { | ||
| return err | ||
| } | ||
|
|
||
| // Add the virtual terminal processing flag to the current mode. | ||
| newMode := mode | enableVirtualTerminalProcessing | ||
|
|
||
| // Try to set the new console mode. | ||
| ret, _, err = procSetConsoleMode.Call(uintptr(handle), uintptr(newMode)) | ||
| if ret == SYS_CALL_FAILURE { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| //go:build windows | ||
|
|
||
| package ansi | ||
|
|
||
| import ( | ||
| "os" | ||
| "syscall" | ||
| "testing" | ||
| "unsafe" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // getConsoleMode is a helper to retrieve the current console mode for a given handle. | ||
| func getConsoleMode(handle syscall.Handle) (uint32, error) { | ||
| var mode uint32 | ||
| ret, _, err := syscall.NewLazyDLL("kernel32.dll").NewProc("GetConsoleMode").Call(uintptr(handle), uintptr(unsafe.Pointer(&mode))) | ||
| if ret == SYS_CALL_FAILURE { | ||
| // Note: err may be non-nil even on success, so we must check ret first. | ||
| return 0, err | ||
| } | ||
| return mode, nil | ||
| } | ||
|
|
||
| // setConsoleMode is a helper to set the console mode for a given handle. | ||
| func setConsoleMode(handle syscall.Handle, mode uint32) error { | ||
| ret, _, err := syscall.NewLazyDLL("kernel32.dll").NewProc("SetConsoleMode").Call(uintptr(handle), uintptr(mode)) | ||
| if ret == SYS_CALL_FAILURE { | ||
| return err | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func TestEnable(t *testing.T) { | ||
| // This test requires a real Windows console. If stdout is redirected (e.g., in some CI/CD | ||
| // environments), GetConsoleMode will fail. In that case, we should skip the test. | ||
| handle := syscall.Handle(os.Stdout.Fd()) | ||
| originalMode, err := getConsoleMode(handle) | ||
| if err != nil && err.Error() != "The handle is invalid." { | ||
| // "The handle is invalid." is the typical error when not in a console. | ||
| // We skip on this specific error. | ||
| t.Skipf("cannot get console mode, skipping test: %v", err) | ||
| } | ||
|
|
||
| // Defer the restoration of the original console mode. | ||
| // This ensures that we don't mess up the terminal for subsequent tests. | ||
| if err == nil { // Only restore if we successfully got the mode. | ||
| defer func() { | ||
| err := setConsoleMode(handle, originalMode) | ||
| require.NoError(t, err, "failed to restore original console mode") | ||
| }() | ||
| } | ||
|
|
||
| // Run the function we want to test. | ||
| err = Enable() | ||
| require.NoError(t, err, "Enable() should not return an error in a real console") | ||
|
|
||
| // After running Enable(), check the console mode again to see if the flag was set. | ||
| newMode, err := getConsoleMode(handle) | ||
| require.NoError(t, err, "failed to get new console mode after enabling ANSI") | ||
|
|
||
| // Assert that the flag for virtual terminal processing is now set. | ||
| flagIsSet := (newMode & enableVirtualTerminalProcessing) != 0 | ||
| require.True(t, flagIsSet, "ENABLE_VIRTUAL_TERMINAL_PROCESSING flag should have been set") | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| package godump | ||
|
|
||
| import ( | ||
| "os" | ||
|
|
||
| "golang.org/x/term" | ||
| ) | ||
|
|
||
| // isTerminal checks if the given file is a terminal using the Go standard library. | ||
| func isTerminal(f *os.File) bool { | ||
| return term.IsTerminal(int(f.Fd())) | ||
| } | ||
Andrei-hub11 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,19 @@ | ||||||||||||||
| package godump | ||||||||||||||
|
|
||||||||||||||
| import ( | ||||||||||||||
| "os" | ||||||||||||||
| "testing" | ||||||||||||||
|
|
||||||||||||||
| "github.com/stretchr/testify/assert" | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| func TestIsTerminal(t *testing.T) { | ||||||||||||||
| t.Run("should return false for a regular file", func(t *testing.T) { | ||||||||||||||
| tmpFile, err := os.CreateTemp("", "test-is-terminal") | ||||||||||||||
| assert.NoError(t, err) | ||||||||||||||
| defer os.Remove(tmpFile.Name()) | ||||||||||||||
| defer tmpFile.Close() | ||||||||||||||
|
Comment on lines
+12
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is enough
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please consider this |
||||||||||||||
|
|
||||||||||||||
| assert.False(t, isTerminal(tmpFile)) | ||||||||||||||
| }) | ||||||||||||||
| } | ||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove this file, and rework the windowsansi package to something like this.
internal/ansi/ansi_windows.go
internal/ansi/ansi_others.go
Then call ansi.Enable from godump when needed. Then you can catch the error returned by the DDL methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if the windows lib require to use init, I would do this
internal/fixansi/ansi_windows.go
And then
In godump, I would use this
I would prefer the first option, or the exiting code currently in the PR