Use color if stdout is a tty #91
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
|
Great idea. I’ve been thinking about doing this. Can you add a changelog entry? Also should we add a test for it? Maybe in the exe integration tests send at at four/stderr to a file and then read it back in and assert no control chars? |
Contributor
Author
|
Should we do this on all location where we pass terminal? Some places it's true and other it's false. |
Collaborator
|
The main interfaces that need it are DeadEnd.call and the script in exe/dead_end (I think). most places in the existing tests are explicitly disabling terminal mode because otherwise we have to strip control characters for assertions. |
It's quite commen for commands use check if the output is a terminal or for and use that to determine if it should print colors.
When you print these codes to an none interactive shell it looks like. This could be build tools integrated in ides or Ci.
```
4 describe Iso20022::Camt053 do
�[0m 223 describe "::Parser", :aggregate_failures do
�[0m❯ 376 �[1;3m fdescribe "DNB cryptic description" do
�[0m❯ 400 �[1;3m end
�[0m❯ 401 �[1;3m end
�[0m 535 end
�[0m 536 end
�[0m
```
Contributor
Author
|
Made some new changes, including changelog. Some other changed you'd like to see? |
Collaborator
|
Thanks, I'll merge and tweak it a bit. I really appreciate your help! Thanks for the contribution ❤️ |
schneems
pushed a commit
that referenced
this pull request
Oct 26, 2021
In #91 the behavior of dead_end is now optimized to output values based on if the io object is a TTY (interactive terminal) or not. This behavior is awesome. The way we were checking default values though introduces a subtle bug, in Ruby `nil` and `false` are usually assumed to have the same/similar behavior (being falsey). This updates the logic to use a `Object.new` value to indicate a default value rather than rely on specific contents. It will reduce the chance the programmer accidentally passes in a value that triggers the default behavior. I also REALLY wanted to find a way to assert the inverse of the test added in "passing --terminal will force color codes" but it turns out it's really hard (if not impossible). I asked on twitter as well https://twitter.com/schneems/status/1452814816456945672. Instead I added some unit tests to at least assert that calling DisplayInvalidBlocks with default args defaulted to `io.isatty`. I also got rid of the describe block for now as there's only one thing in there. It makes diffs a bit more muddled if the contents of the tests get moved around. Personal preference.
schneems
pushed a commit
that referenced
this pull request
Oct 26, 2021
In #91 the behavior of dead_end is now optimized to output values based on if the io object is a TTY (interactive terminal) or not. This behavior is awesome. The way we were checking default values though introduces a subtle bug, in Ruby `nil` and `false` are usually assumed to have the same/similar behavior (being falsey). This updates the logic to use a `Object.new` value to indicate a default value rather than rely on specific contents. It will reduce the chance the programmer accidentally passes in a value that triggers the default behavior. I also REALLY wanted to find a way to assert the inverse of the test added in "passing --terminal will force color codes" but it turns out it's really hard (if not impossible). I asked on twitter as well https://twitter.com/schneems/status/1452814816456945672. Instead I added some unit tests to at least assert that calling DisplayInvalidBlocks with default args defaulted to `io.isatty`. I also got rid of the describe block for now as there's only one thing in there. It makes diffs a bit more muddled if the contents of the tests get moved around. (Personal preference)
schneems
added a commit
that referenced
this pull request
Oct 26, 2021
In #91 the behavior of dead_end is now optimized to output values based on if the io object is a TTY (interactive terminal) or not. This behavior is awesome. The way we were checking default values though introduces a subtle bug, in Ruby `nil` and `false` are usually assumed to have the same/similar behavior (being falsey). This updates the logic to use a `Object.new` value to indicate a default value rather than rely on specific contents. It will reduce the chance the programmer accidentally passes in a value that triggers the default behavior. I also REALLY wanted to find a way to assert the inverse of the test added in "passing --terminal will force color codes" but it turns out it's really hard (if not impossible). I asked on twitter as well https://twitter.com/schneems/status/1452814816456945672. Instead I added some unit tests to at least assert that calling DisplayInvalidBlocks with default args defaulted to `io.isatty`. I also got rid of the describe block for now as there's only one thing in there. It makes diffs a bit more muddled if the contents of the tests get moved around. (Personal preference)
schneems
added a commit
that referenced
this pull request
Oct 26, 2021
In #91 the behavior of dead_end is now optimized to output values based on if the io object is a TTY (interactive terminal) or not. This behavior is awesome. The way we were checking default values though introduces a subtle bug, in Ruby `nil` and `false` are usually assumed to have the same/similar behavior (being falsey). This updates the logic to use a `Object.new` value to indicate a default value rather than rely on specific contents. It will reduce the chance the programmer accidentally passes in a value that triggers the default behavior. I also REALLY wanted to find a way to assert the inverse of the test added in "passing --terminal will force color codes" but it turns out it's really hard (if not impossible). I asked on twitter as well https://twitter.com/schneems/status/1452814816456945672. Instead I added some unit tests to at least assert that calling DisplayInvalidBlocks with default args defaulted to `io.isatty`. I also got rid of the describe block for now as there's only one thing in there. It makes diffs a bit more muddled if the contents of the tests get moved around. (Personal preference)
schneems
added a commit
that referenced
this pull request
Oct 26, 2021
In #91 the behavior of dead_end is now optimized to output values based on if the io object is a TTY (interactive terminal) or not. This behavior is awesome. The way we were checking default values though introduces a subtle bug, in Ruby `nil` and `false` are usually assumed to have the same/similar behavior (being falsey). This updates the logic to use a `Object.new` value to indicate a default value rather than rely on specific contents. It will reduce the chance the programmer accidentally passes in a value that triggers the default behavior. I also REALLY wanted to find a way to assert the inverse of the test added in "passing --terminal will force color codes" but it turns out it's really hard (if not impossible). I asked on twitter as well https://twitter.com/schneems/status/1452814816456945672. Instead I added some unit tests to at least assert that calling DisplayInvalidBlocks with default args defaulted to `io.isatty`. I also got rid of the describe block for now as there's only one thing in there. It makes diffs a bit more muddled if the contents of the tests get moved around. (Personal preference)
schneems
added a commit
that referenced
this pull request
Oct 26, 2021
In #91 the behavior of dead_end is now optimized to output values based on if the io object is a TTY (interactive terminal) or not. This behavior is awesome. The way we were checking default values though introduces a subtle bug, in Ruby `nil` and `false` are usually assumed to have the same/similar behavior (being falsey). This updates the logic to use a `Object.new` value to indicate a default value rather than rely on specific contents. It will reduce the chance the programmer accidentally passes in a value that triggers the default behavior. I also REALLY wanted to find a way to assert the inverse of the test added in "passing --terminal will force color codes" but it turns out it's really hard (if not impossible). I asked on twitter as well https://twitter.com/schneems/status/1452814816456945672. Instead I added some unit tests to at least assert that calling DisplayInvalidBlocks with default args defaulted to `io.isatty`. I also got rid of the describe block for now as there's only one thing in there. It makes diffs a bit more muddled if the contents of the tests get moved around. (Personal preference)
Collaborator
|
I opened up #92 with some minor changes to your logic. I would love it if you could review that PR and let me know if you've got any feedback or questions. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It's quite commen for commands use check if the output is a terminal or for and use that to determine if it should print colors.
When you print these codes to an none interactive shell it looks like. This could be build tools integrated in ides or Ci.
This fixes my case. It would proabaly be better if I spent a little more time and check each place its written. Like at https://github.com/zombocom/dead_end/blob/2e3807649d7f999cc3e42ebba0792f8701a79373/lib/dead_end/display_invalid_blocks.rb#L50 we could check .@io
What's you think? I wanted to open a minimal PR so check if you have any plans for this