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

Added colors to file names and line+col numbers in clear-text reporter #1185

Merged
merged 8 commits into from
Oct 28, 2018
Merged

Added colors to file names and line+col numbers in clear-text reporter #1185

merged 8 commits into from
Oct 28, 2018

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Oct 15, 2018

Same cyan & yellow as TypeScript. It seems to work well there. Removes the extra lines of output as an opt-out behavior to make output more concise.

Starts on #975 (fancy emojis can come next!)

Before:

image

After:

image

Same cyan & yellow as I added to TypeScript. It seems to work well there. Starts on 975 (fancy emojis can come next!)
@ghost ghost added the 🔎 Needs review label Oct 15, 2018
@JoshuaKGoldberg
Copy link
Contributor Author

JoshuaKGoldberg commented Oct 15, 2018

I'm a little lost trying to get these unit tests to work with chalk 😕... if someone could help, I'd appreciate it! Debugged and realized I was off by one. Colored printing was fun.

@nicojs
Copy link
Member

nicojs commented Oct 15, 2018

Hmm looks better I think. I'll have a look at the tests.

@simondel what do you think? It removes the names of the tests that ran, but do we really need them? It's a nice feature, but it also takes up a lot of space.

Maybe we can have 1 sentence:

Ran 3/24 tests for this mutant

Ran all tests for this mutant

EDIT:

Apparently it is configurable now? Very cool, let me check it out first :)

@ghost ghost assigned nicojs Oct 15, 2018
@JoshuaKGoldberg
Copy link
Contributor Author

I also wonder if we really need the Mutant survived! complaint on top of each file. How about a 12/345 mutants survived! once before the list?

@nicojs
Copy link
Member

nicojs commented Oct 15, 2018

It is a lot of repeated text. It would be awesome if could at a remark in front of the mutator:

1. [Survived] StringLiteral
/tmp/cmp/success.component.js:5:5
-    controllerAs: 'cpt'
+    controllerAs: ''
2. [No coverage] BinaryOperator
/tmp/cmp/success.component.js:10:5
-    1+1
+    1-1

I would just remove the Mutant survived! entirely. I'm pretty sure it is remnant from the original code base, when @simondel was still a student :). A blast from the past.

@JoshuaKGoldberg
Copy link
Contributor Author

JoshuaKGoldberg commented Oct 15, 2018

If you don't mind, I think we could also remove the Mutator: *Mutator message too. Doesn't seem too useful to end users, and we can generally figure it out from the change. Removed in the latest commit.

Edit: Just re-read and realized I'd misinterpreted your last comment, nicojs. I can make that change if you'd like! Will wait for input.

@simondel
Copy link
Member

RIP Mutant Survived!
2015 - 2018

@nicojs
Copy link
Member

nicojs commented Oct 16, 2018

If you don't mind, I think we could also remove the Mutator: *Mutator message too.

This is actually useful info. If you want to disable a mutator, you need to know the name.

@nicojs
Copy link
Member

nicojs commented Oct 18, 2018

I'm thinking of adding a global option for allowColor. Instead of one specific for the clear text reporter. Might be useful for the log4js formatting as well. @JoshuaKGoldberg is it OK if I move it to a separate issue?

@nicojs
Copy link
Member

nicojs commented Oct 18, 2018

How about this?

image

@JoshuaKGoldberg
Copy link
Contributor Author

global option for allowColor

That sounds nice... though what if we want to have one reporter in color and another without? Would the allowColor set what the default is, as overridden by reporters?

I'll update this PR for the 105. [NoCoverage] Block, and if your separate issue / branch is different and this PR gets closed, I won't be offended in the slightest. 😊

@nicojs
Copy link
Member

nicojs commented Oct 28, 2018

That sounds nice... though what if we want to have one reporter in color and another without?

What would that use case be? The only reason I see for disabling the colors is for build servers.

@nicojs nicojs merged commit a49829b into stryker-mutator:master Oct 28, 2018
@ghost ghost removed the 🔎 Needs review label Oct 28, 2018
@nicojs
Copy link
Member

nicojs commented Oct 28, 2018

@JoshuaKGoldberg I've created a separate issue for it. Could you explain it further there? #1218

@JoshuaKGoldberg JoshuaKGoldberg deleted the improved-cli-colors branch October 28, 2018 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants