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

made colored output more consistent #1706

Merged
merged 3 commits into from Nov 8, 2018

Conversation

Projects
None yet
2 participants
@emekoi
Copy link
Contributor

emekoi commented Nov 7, 2018

second attempt.

@andrewrk
Copy link
Member

andrewrk left a comment

👍

@@ -71,6 +71,19 @@ pub fn getSelfDebugInfo() !*DebugInfo {
}
}

pub fn supportsAnsi(file: os.File) bool {

This comment has been minimized.

Copy link
@andrewrk

andrewrk Nov 7, 2018

Member

I think we already have this function - std.os.isTty

This comment has been minimized.

Copy link
@emekoi

emekoi Nov 8, 2018

Author Contributor

the problem is that std.os.isTty returns true for the windows console even though it does't support ansi escape codes, so i needed a different function.

This comment has been minimized.

Copy link
@andrewrk

andrewrk Nov 8, 2018

Member

I see. I think this is still not quite what we want. This is testing if GetConsoleMode fails:

If the function succeeds, the return value is nonzero.
If the function fails, the return value is zero. To get extended error information, call GetLastError.

It's strange to conclude that a File supports ANSI because we get an error for getting the console mode.
I think instead we would want to return true for Windows if windowsIsCygwinPty return true.

I would also propose to put this function right next to isTty and accept a FileHandle (same as isTty) rather than a File. Maybe sometime in the future we refactor these both to use File; maybe we don't. But I think they should be together.

One more proposal: name it supportsAnsiEscapeCodes. Reasoning:

  • descriptive; avoids confusion from people thinking a file would support ANSI character encodings or something like that.
  • unlikely to be called many times; a longer name is fine

This comment has been minimized.

Copy link
@emekoi

emekoi Nov 8, 2018

Author Contributor

i was looking at windowsIsCygwinPty and i was wondering why we return true when GetFileInformationByHandleEx fails. when i change the return value to false there everything works as expected.

This comment has been minimized.

Copy link
@andrewrk

andrewrk Nov 8, 2018

Member

Hmm that does seem like a mistake. I think we should change it to false.

This comment has been minimized.

Copy link
@emekoi

emekoi Nov 8, 2018

Author Contributor

would you mind if i moved these functions into a terminal submodule under os?

This comment has been minimized.

Copy link
@andrewrk

andrewrk Nov 8, 2018

Member

Go for it 👍

This comment has been minimized.

Copy link
@emekoi

emekoi Nov 8, 2018

Author Contributor

i'll move that to a separate pull request as i plan to add more functionality, like moving the cursor and querying terminal size.

This comment has been minimized.

Copy link
@andrewrk

andrewrk Nov 8, 2018

Member

Sounds good. So this PR is ready then?

This comment has been minimized.

Copy link
@emekoi

emekoi Nov 8, 2018

Author Contributor

yup.

Show resolved Hide resolved std/debug/index.zig Outdated

@andrewrk andrewrk merged commit 8e69a18 into ziglang:master Nov 8, 2018

1 check was pending

ziglang.zig in progress
Details
@andrewrk

This comment has been minimized.

Copy link
Member

andrewrk commented Nov 8, 2018

Thanks!

@emekoi emekoi deleted the emekoi:windows-ansi branch Nov 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.