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

Add fixable status to verbose and github formatters #6183

Merged
merged 5 commits into from Jun 29, 2022

Conversation

ybiquitous
Copy link
Member

Which issue, if any, is this issue related to?

Ref: #6099

Is there anything in the PR that needs further explanation?

In this pull request, I try showing fixable metadata with the verbose and github formatters.
But I’m not sure whether this addition is really useful. I would be happy if any feedback.

Demo on localhost:

$ (cd scripts ; ./visual.sh)
...

4 errors found
 color-no-invalid-hex: 1
 import-notation: 1 (fixable)
 selector-max-class: 1
 unit-disallowed-list: 1

1 warning found
 custom-property-pattern: 1

image

@jeddy3
Copy link
Member

jeddy3 commented Jun 28, 2022

I like the addition, especially to the verbose formatter as I feel that those users want as much information as possible. The thing is, not all problems can be fixed by a rule; sometimes we fix only for certain options or we ignore some edge cases. Do we need to communicate that here? For example, by using "maybe fixable" instead of "fixable":

import-notation: 1 (maybe fixable)

People may be confused if they see "fixable", run "--fix" and then still see the problem listed.

@ybiquitous
Copy link
Member Author

@jeddy3 Thanks for the feedback. Your concern and suggestion make sense to me. 👍🏼
I've pushed the commit 973935c to add the maybe word to messages.

@ybiquitous
Copy link
Member Author

ybiquitous commented Jun 28, 2022

In addition, what if showing the following message (only when fixable problems are found), for example?

You may fix some problems with the "--fix" option.

image

Is this too verbose?

Code change
diff --git a/lib/formatters/verboseFormatter.js b/lib/formatters/verboseFormatter.js
index 2c3c9c55b..11742ab1f 100644
--- a/lib/formatters/verboseFormatter.js
+++ b/lib/formatters/verboseFormatter.js
@@ -55,6 +55,7 @@ module.exports = function verboseFormatter(results, returnValue) {
 		output += '\n0 problems found\n';
 	} else {
 		const warningsBySeverity = groupBy(warnings, (w) => w.severity);
+		let fixableProblemsFound = false;
 
 		/**
 		 * @param {Severity} severity
@@ -75,11 +76,17 @@ module.exports = function verboseFormatter(results, returnValue) {
 				const fixable = meta && meta.fixable ? ' (maybe fixable)' : '';
 
 				output += dim(` ${ruleLink(rule, meta)}: ${list.length}${fixable}\n`);
+
+				if (!fixableProblemsFound && meta && meta.fixable) fixableProblemsFound = true;
 			}
 		};
 
 		printProblems('error');
 		printProblems('warning');
+
+		if (fixableProblemsFound) {
+			output += yellow('\nYou may fix some problems with the "--fix" option.\n');
+		}
 	}
 
 	return `${output}\n`;

@jeddy3 jeddy3 changed the title Show fixable metadata with verbose and github formatters Add fixable status to verbose and github formatters Jun 29, 2022
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

In addition, what if showing the following message (only when fixable problems are found)

👍

@jeddy3 jeddy3 mentioned this pull request Jun 29, 2022
6 tasks
@ybiquitous
Copy link
Member Author

Thanks for the feedback!

In addition, what if showing the following message (only when fixable problems are found)

Changed via b2c92ef.

@ybiquitous ybiquitous merged commit e70c93d into main Jun 29, 2022
@ybiquitous ybiquitous deleted the show-fixable-with-formatters branch June 29, 2022 13:58
@ybiquitous
Copy link
Member Author

Changelog entry added:

  • Added: fixable status to verbose and github formatters (#6183).

@Mouvedia
Copy link
Contributor

Mouvedia commented Jul 13, 2022

sometimes we fix only for certain options or we ignore some edge cases

@jeddy3 Should we enforce the addition of one skipped test per edge case not covered?
That would permit to surface whether a rule is always fixable or not.

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

Successfully merging this pull request may close these issues.

None yet

3 participants