-
Notifications
You must be signed in to change notification settings - Fork 503
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
Added support for customizing the output color for #582. #585
Added support for customizing the output color for #582. #585
Conversation
@@ -128,9 +128,10 @@ class KtlintCommandLine { | |||
|
|||
@Option( | |||
names = ["--color"], | |||
description = ["Make output colorful"] | |||
description = ["Make output colorful. To specify the color, use --COLOR=RED"], |
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 think it's better to specify lowercase --color
here, because picocli is case sensitive. You'll end up with UnmatchedArgumentException
otherwise.
private fun getColor(colorInput: String?): Color? { | ||
return when (colorInput) { | ||
"null" -> null | ||
else -> Color.values().firstOrNull { it.name == colorInput } ?: Color.DARK_GRAY |
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.
Not sure, maybe it's better not to fallback to gray color here, but behave like the value wasn't supplied? Or maybe even throw an exception that the argument is not matched.
@rom4ek Thanks for the feedback! I think those were good changes and I've made the updates. Let me know what you think of this. Also if you want me to rebase and squash these commits into one I'd be happy to once we resolve the reviews. |
@AdamMc331 not that I pretended to review, but lgtm, I think it's a great improvement :) |
@AdamMc331 thanks for the PR! Can you see what happens if you pass in a color for |
I get output like this, but I don't see anything colored red. Should I expect to? I'm not familiar with printAST. I just realized that causes an interesting problem with this though. If I were to run just |
) | ||
var color: Boolean = false | ||
var color: String? = null |
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.
Changing type here will be a breaking change.
I would propose to introduce additional parameter --console-main-color
(short -C
), that will be strongly typed Color
: https://picocli.info/#_enum_types with default value Color.NO_COLOR
. Additionally deprecate --color
parameter.
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.
@Tapchicoma Do you think I may still run into the problem mentioned here where if I did --console-main-color printAST
it would be interpreted as the param name? #585 (comment)
I can test later to be sure, I'm still learning picocli.
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.
This particular problem comes from arity = "0..1"
that allows to not have option value: https://picocli.info/#_arity. In the case above printAST
is parsed as optional option value and not as subcommand.
My proposal for --console-main-color
is to have mandatory option value (basically arity = "1"
)
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.
Okay. Should be easy enough. I'm a little iffy about deprecating --color
. The other alternative could be to introduce a new parameter, --color-name
but then I think that's kind of annoying to ask the user to specify two parameters and do --color --color-name RED
.
Which would you prefer?
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.
Maybe yes, let's leave --color
option that would just enable output coloring with default color (RED
?) and --color-name
that could be used to change color value.
c46cd9c
to
ed35f20
Compare
Okay I've updated this to use a second parameter after all, which should be a non breaking change but provide people with this additional functionality. What is the rule when it comes to changelog/README? Should I update those files to account for this change too, or will that happen closer to the release process? |
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.
LGTM! Thank you for your contribution.
Changelog will be updated before release.
} catch (iae: IllegalArgumentException) { | ||
// Expected case | ||
} catch (e: Exception) { | ||
fail("Expected IllegalArgumentException but was: $e") |
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.
Nit: you don't need to add Exception
catch block. If code will throw uncaught exception - test will fail.
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.
Ooo whoops. Want me to fix?
import org.junit.Assert.fail | ||
import org.junit.Test | ||
|
||
class PlainReporterProviderTest { |
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.
Thank you for adding tests 👍
names = ["--color-name"], | ||
description = ["Customize the output color"] | ||
) | ||
var colorName: String = Color.DARK_GRAY.name |
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.
Would be nice to use here enum type, but it will require a lot of changes :(
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 agree and thought the same but I wasn't sure if I could reference an enum as a command line argument? I assumed it had to be a string. If you know something I don't - let me know and I can try to adjust.
Whoops replied while merging happened. Let me know if anything was glaring and we can talk about it as a future follow up. Thanks for reviewing! |
Summary
This resolves the issue discussed here, that the default DARK_GRAY background color was hard to read against dark terminals: #582
Usage
I made sure this was a non breaking change by allowing the current usage of KtLint to behave the way it does now. For example:
How it was tested
I've added unit tests to
PlainReporterTest
to validate the colored output. I also addedPlainReporterProviderTest
which ensures the input for a color maps to the expected output.I also ran this manually. Here is KtLint with no command args:
Here it is with
--color
:Here it is with
--color --colorName="RED"
: