Disable colors on Windows OS #1701

remal · 2019-06-16T17:00:12Z

This is simple and a bit dirty fix for #1694 - remove color chars in console output for Windows OS.

IMHO this PR is less preferable than #1702

Signed-off-by: Semyon Levin <levin.semen@gmail.com>

codecov-io · 2019-06-16T17:18:07Z

Codecov Report

Merging #1701 into master will decrease coverage by 3.22%.
The diff coverage is 57.14%.

@@             Coverage Diff              @@
##             master    #1701      +/-   ##
============================================
- Coverage     79.14%   75.91%   -3.23%     
- Complexity     1760     1856      +96     
============================================
  Files           285      328      +43     
  Lines          4943     5527     +584     
  Branches        908     1010     +102     
============================================
+ Hits           3912     4196     +284     
- Misses          541      791     +250     
- Partials        490      540      +50

Impacted Files	Coverage Δ	Complexity Δ
.../gitlab/arturbosch/detekt/cli/console/Colorizer.kt	`76.92% <57.14%> (-23.08%)`	`0 <0> (ø)`
.../gitlab/arturbosch/detekt/rules/empty/EmptyRule.kt	`86.66% <0%> (-4.25%)`	`10% <0%> (+2%)`
...ab/arturbosch/detekt/formatting/KtLintMultiRule.kt	`88.88% <0%> (-3.97%)`	`9% <0%> (+3%)`
...kotlin/io/gitlab/arturbosch/detekt/api/Findings.kt	`0% <0%> (ø)`	`0% <0%> (?)`
...tlin/io/gitlab/arturbosch/detekt/api/YamlConfig.kt	`70.83% <0%> (ø)`	`7% <0%> (?)`
...in/io/gitlab/arturbosch/detekt/api/SplitPattern.kt	`75% <0%> (ø)`	`10% <0%> (?)`
...ekt/sample/extensions/rules/TooManyFunctionsTwo.kt	`0% <0%> (ø)`	`0% <0%> (?)`
...io/gitlab/arturbosch/detekt/api/CompositeConfig.kt	`60% <0%> (ø)`	`4% <0%> (?)`
...n/kotlin/io/gitlab/arturbosch/detekt/api/Entity.kt	`50% <0%> (ø)`	`1% <0%> (?)`
...n/io/gitlab/arturbosch/detekt/api/ProjectMetric.kt	`0% <0%> (ø)`	`0% <0%> (?)`
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0dd2eba...c119511. Read the comment docs.

schalkms

Thanks for your time valuable feedback and your two proposals for this issue!!
I favor this approach, because it's simpler and doesn't require a 3rd party library as a dependency.
I have some questions.

What happens if this is executed in the Cygwin shell on Windows? The shell supports ANSI coloring.
What happens if this is executed in the Windows PowerShell?

CC @marschwar

schalkms · 2019-06-16T18:45:39Z

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/console/Colorizer.kt

@@ -12,7 +12,12 @@ private data class Color(private val value: Byte) {
        get() = "$ESC[${value}m"
 }

-private fun CharSequence.colorized(color: Color) = "${color.escapeSequence}$this${RESET.escapeSequence}"
+private val isColoredOutputSupported: Boolean = !System.getProperty("os.name", "").contains("win", true)


What happens if the OS is Darwin? Then this variable is true.
The check should be written with startsWith to be safer.

!System.getProperty("os.name", "").startsWith("Windows")

remal · 2019-06-17T09:07:41Z

@schalkms The Idea behind checking if the property contains win substring is that it's better to turn off colorizing than add colors on a shell without colors support.

Actually I don't see any major drawbacks with adding Jansi dependency to detekt-cli module. I suppose those who don't need this library, probably, won't use detekt-cli.

Also, as I understand, all checks you mentioned are supported by Jansi.

BTW IDEA correctly displays colorized output by Jansi.

schalkms · 2019-06-17T18:01:22Z

The Idea behind checking if the property contains win substring is that it's better to turn off colorizing than add colors on a shell without colors support.

I fully agree. However, .contains("win") could match operating systems, which are not Windows. Thus, we need to change it to .startsWith("Windows") to be safer here.
Apache System Utils also does it this way.

Actually I don't see any major drawbacks with adding Jansi dependency to detekt-cli module

Jansi uses JNI on Windows to support coloring terminal output.
I'd say for detekt's simple usage it's not necessary to import this library.

Also, as I understand, all checks you mentioned are supported by Jansi.

I tried it out on Windows with the following shells.

Cygwin - supports it natively without any hacks
Cmder supports cmd and unix shells - works out-of-the-box
Cmd + Powershell - do not support coloring the terminal output without any hacks (as expected).

BTW IDEA correctly displays colorized output by Jansi.

True, because IntelliJ only emulates the default cmd on Windows, if not configured otherwise.

marschwar · 2019-06-17T18:38:13Z

I agree with @schalkms. I don't really see the need for adding an additional library just for this. It's far from being a core feature of detekt.

I could live with contains or startsWith, but in my opinion we should check for the string windows as it seems to be common practice.

See:
https://github.com/sonatype/plexus-interpolation/blob/16f8c521c60021fbac3295f0ed10ba5bc6cfdc9a/src/main/java/org/codehaus/plexus/interpolation/os/Os.java#L308
https://github.com/apache/commons-lang/blob/dc2953e5e41f0d5bf069349bbd7c6fe26f3362e1/src/main/java/org/apache/commons/lang3/SystemUtils.java#L1355

marschwar · 2019-06-17T18:44:03Z

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/console/Colorizer.kt

+    "${color.escapeSequence}$this${RESET.escapeSequence}"
+} else {
+    this.toString()
+}


Minor nitpick: In my opinion CharSequence.colorized(color: Color) should itself return a CharSequence if possible.

arturbosch · 2019-06-18T05:35:36Z

Thanks @remal @schalkms @marschwar for further investigations. So lets go with this pr then.

Signed-off-by: Semyon Levin <levin.semen@gmail.com>

remal · 2019-06-18T16:57:55Z

Guys, please take a look.

Signed-off-by: Semyon Levin <levin.semen@gmail.com>

schalkms · 2019-06-18T17:12:04Z

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/console/Colorizer.kt

-fun CharSequence.decolorized() = this.replace(escapeSequenceRegex, "")
+fun String.red(): String = colorized(RED).toString()
+fun String.yellow(): String = colorized(YELLOW).toString()
+fun String.decolorized(): String = this.replace(escapeSequenceRegex, "")


Why do we have the same extension method String and CharSequence? Did I miss something?

Because @marschwar asked to make CharSequence.colorized(color: Color) to return CharSequence. But there are some places where String is expected, so it would be more convenient to have a method that returns String. I suggest leaving only one variant that returns String. WDYT?

Sorry to cause such a confusion. I had not actually made the change myself so I did not realize there were usages where a String is expected to be returned. My point was that the extension function should return the same type if possible.
Please keep fun String.colorized(color: Color): String only. AFAIK we don't need the extension function to be on CharSequence.

Signed-off-by: Semyon Levin <levin.semen@gmail.com>

* Disable colors on Windows OS Signed-off-by: Semyon Levin <levin.semen@gmail.com> * CharSequence.colorized returns CharSequence Signed-off-by: Semyon Levin <levin.semen@gmail.com> * Is Windows check is changed Signed-off-by: Semyon Levin <levin.semen@gmail.com> * code formatting Signed-off-by: Semyon Levin <levin.semen@gmail.com> * simplification Signed-off-by: Semyon Levin <levin.semen@gmail.com>

Disable colors on Windows OS

33a0cd9

Signed-off-by: Semyon Levin <levin.semen@gmail.com>

remal mentioned this pull request Jun 16, 2019

Use jansi for colorized console output #1702

Closed

schalkms reviewed Jun 16, 2019

View reviewed changes

marschwar reviewed Jun 17, 2019

View reviewed changes

arturbosch added this to the 1.0.0 milestone Jun 18, 2019

remal added 2 commits June 18, 2019 19:54

CharSequence.colorized returns CharSequence

11a2622

Signed-off-by: Semyon Levin <levin.semen@gmail.com>

Is Windows check is changed

2ef4275

Signed-off-by: Semyon Levin <levin.semen@gmail.com>

code formatting

09e41cc

Signed-off-by: Semyon Levin <levin.semen@gmail.com>

schalkms reviewed Jun 18, 2019

View reviewed changes

simplification

c119511

Signed-off-by: Semyon Levin <levin.semen@gmail.com>

schalkms approved these changes Jun 19, 2019

View reviewed changes

arturbosch merged commit a6f4335 into detekt:master Jun 19, 2019

remal deleted the disable-colors-on-windows branch June 28, 2019 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable colors on Windows OS #1701

Disable colors on Windows OS #1701

remal commented Jun 16, 2019 •

edited

Loading

codecov-io commented Jun 16, 2019 •

edited

Loading

schalkms left a comment

schalkms Jun 16, 2019

remal Jun 18, 2019

remal commented Jun 17, 2019 •

edited

Loading

schalkms commented Jun 17, 2019 •

edited

Loading

marschwar commented Jun 17, 2019

marschwar Jun 17, 2019 •

edited

Loading

remal Jun 18, 2019

arturbosch commented Jun 18, 2019

remal commented Jun 18, 2019

schalkms Jun 18, 2019

remal Jun 18, 2019

marschwar Jun 18, 2019

remal Jun 19, 2019

Disable colors on Windows OS #1701

Disable colors on Windows OS #1701

Conversation

remal commented Jun 16, 2019 • edited Loading

codecov-io commented Jun 16, 2019 • edited Loading

Codecov Report

schalkms left a comment

Choose a reason for hiding this comment

schalkms Jun 16, 2019

Choose a reason for hiding this comment

remal Jun 18, 2019

Choose a reason for hiding this comment

remal commented Jun 17, 2019 • edited Loading

schalkms commented Jun 17, 2019 • edited Loading

marschwar commented Jun 17, 2019

marschwar Jun 17, 2019 • edited Loading

Choose a reason for hiding this comment

remal Jun 18, 2019

Choose a reason for hiding this comment

arturbosch commented Jun 18, 2019

remal commented Jun 18, 2019

schalkms Jun 18, 2019

Choose a reason for hiding this comment

remal Jun 18, 2019

Choose a reason for hiding this comment

marschwar Jun 18, 2019

Choose a reason for hiding this comment

remal Jun 19, 2019

Choose a reason for hiding this comment

remal commented Jun 16, 2019 •

edited

Loading

codecov-io commented Jun 16, 2019 •

edited

Loading

remal commented Jun 17, 2019 •

edited

Loading

schalkms commented Jun 17, 2019 •

edited

Loading

marschwar Jun 17, 2019 •

edited

Loading