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

Colorize output #104

Merged
merged 5 commits into from
Dec 18, 2017
Merged

Colorize output #104

merged 5 commits into from
Dec 18, 2017

Conversation

schumacher-m
Copy link
Contributor

@schumacher-m schumacher-m commented Dec 7, 2017

I want to bounce some ideas if it makes sense to enhance the tool to visually give (more) feedback on passing or broken migrations (similar to most test frameworks red->green schema)
I usually let the tool run and continue on something else - i still notice that the log is moving and for me it helped that it spitted out a red text on a failure as this usually gets my attention immediately.

In our internal version of mybatis-migrations we also red-color the exception to separate them from the migration content that is also printed out and made the tool less chatty by removing the status lines. If you run many and large migrations it helps to quickly jump to a red part and see what was wrong as you tend to lose where the error started.

Nonetheless. I stripped our version down to just printing the SUCCESS or FAILURE, exceptions in different colors to have a small increment and hope we can just build-up from there.

Activate it by adding "--color" as a parameter.

(pom.xml change was done to build locally)

@simonetripodi
Copy link
Member

simonetripodi commented Dec 7, 2017

This is IMHO a nice idea, I am +1 to apply the Pull Request

@schumacher-m I think you can make the ConsoleColors as an interface rather just a regular class, since it declares constants only and, in order to apply the commit, you need to revert the version in the pom.

Very well done!

@schumacher-m
Copy link
Contributor Author

Alright. I rebased and dropped the pom change.

@simonetripodi
Copy link
Member

@schumacher-m if you don't want to make the _ ConsoleColors_ class as an interface, drop the public modifier and implement the default constructor as private since it doesn't have to be instantiated, OTOH it looks good - any chance you can attach a screenshot for the audience, please?

@schumacher-m
Copy link
Contributor Author

I'm not against the interface change - just doing this work thing and I'm bad at multitasking ;)
I'll prepare the screenshot in the evening.

@simonetripodi
Copy link
Member

awesome @schumacher-m , just awesome, thanks for this great addition 👍

@schumacher-m
Copy link
Contributor Author

schumacher-m commented Dec 7, 2017

Here is an example:

colorized_error
colorized_success

@simonetripodi
Copy link
Member

Shiny like a sunshine 👍

I personally would like to have this behaviour enabled by default, can be suppressed via an option, i.e. --no-highlight (highlight is a best therm in such contexts, IMHO)

What other people think? @cbegin ?

@hazendaz
Copy link
Member

hazendaz commented Dec 9, 2017

👍

@harawata
Copy link
Member

Looks nice. Thank you, @schumacher-m !

Regarding the default, I'm fine with either, but no-color (or no-highlight) might be safer.
To avoid typing it every time, I'll make it configurable in the properties file later.

@schumacher-m
Copy link
Contributor Author

(Not sure why it complained about the formatting)

Yes. I think it is more compatible to have it opt-in. I was initially thinking that the added color might break some scripts if they for example grep for the whole success string - might be too constructed though.

Enabling this in the prop file would be ok.


public class CommandLine {
public class CommandLine implements ConsoleColors {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good @schumacher-m but CommandLine implementing ConsoleColors just to bring in the colors is a bit odd. Why not just static imports if you are that concerned about the colors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well initially it was just a class I've imported (see: 6be27e9)

In the end I was indifferent about either way. What would you suggest?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

importing it makes more sense. the public static final are unnecessary anyways as interface values are already public final and static by default but you already made that change. So it's just adding the static imports or quantifying it.
In the future I would love to see the console writing be split out into a strategy so that you can use color console/logging as a strategy across not just the final error and status lines but this is a great feature on it's own.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, I mean to give colors to warnings or pre/post hooks etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Let me switch it back to just a class for now. We can continue on the output. I also like to make it less maven-esque (or at least define some sort of different formatter)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some confusion here, I meant keep it an interface but don't implement it.

@schumacher-m
Copy link
Contributor Author

So I reverted the interface change - probably best to squash this PR. I only see the formatting conflicts on github.

@schumacher-m
Copy link
Contributor Author

So after some confusion I rebased everything (also with latest master in upstream) and here it is again ;)

@schumacher-m
Copy link
Contributor Author

schumacher-m commented Dec 18, 2017

In the future I would love to see the console writing be split out into a strategy so that you can use color console/logging as a strategy across not just the final error and status lines but this is a great feature on it's own.

I'd like to dive into it - since this is somewhat the same direction I wanted to go with this.
Shall we just put it as an issue for better discussion?

@h3adache
Copy link
Member

That would be great @schumacher-m! Thanks for the work here!

@h3adache h3adache merged commit 9b22646 into mybatis:master Dec 18, 2017
@simonetripodi
Copy link
Member

thanks for the awesome work @schumacher-m , much more than appreciated!

thanks @h3adache for merging the PR - I love to see this modification checked-in our codebase! 👍

@harawata harawata added this to the 3.3.2 milestone Dec 25, 2017
harawata added a commit that referenced this pull request Dec 25, 2017
harawata added a commit that referenced this pull request Jan 31, 2018
@harawata
Copy link
Member

Hi all,
With the latest SNAPSHOT, it's now possible to specify color=true in migration.properties instead of specifying --color every time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants