Support error severity screening option #129

yamarkz · 2023-03-22T14:27:05Z

Custom Lint is a cool tool and makes my project better.
A few additional features would make the operation easier. This PR will add an option to screen Error Severity.

I implemented this issue on my own since it was a separate issue from #101 .

Any thoughts or policies? If not, I would be happy to have a positive consideration for the merge based on this PR.
My project is going to have a constructive Custom Lint maintenance once this change is in place.

docs-page · 2023-03-22T14:27:08Z

To view this pull requests documentation preview, visit the following URL:

docs.custom-lint.dev/~129

Documentation is deployed and generated using docs.page.

vercel · 2023-03-22T14:27:09Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
custom-lint-website	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 29, 2023 7:34am

CLAassistant · 2023-03-22T14:27:15Z

All committers have signed the CLA.

yamarkz · 2023-03-22T14:29:58Z

@rrousselGit please review

yamarkz · 2023-04-08T09:03:36Z

@rrousselGit

What do you think? If you do not accept the changes, please close it.

packages/custom_lint/lib/custom_lint.dart

rrousselGit · 2023-04-19T12:04:51Z

packages/custom_lint/lib/custom_lint.dart

-      '  ${_relativeFilePath(error.location.file, workingDirectory)}:${error.location.startLine}:${error.location.startColumn}'
-      ' • ${error.message} • ${error.code}',
-    );
+    hasErrors |= error.severity == AnalysisErrorSeverity.ERROR;


Don't use |=. This is the | operator, not ||.
So the == would be executed even though it's unnecessary.

fixed commit 36140d7

rrousselGit · 2023-04-19T12:06:55Z

packages/custom_lint/lib/custom_lint.dart

+
+  if (hasErrors) {
+    exitCode = 1;
+    return;


If we're going to use "return", there's no need for the for loop above to iterate over all errors. It probably should stop as soon as one unignored error/warning/info is found

fixed commit 36140d7

I didn't take enough considerations for code efficiency

rrousselGit · 2023-04-19T12:07:25Z

packages/custom_lint/test/cli_process_test.dart

@@ -72,7 +72,7 @@ void main() {
  test_app2/lib/main2.dart:1:6 • Hello world • hello_world
 ''',
    );
-    expect(process.exitCode, 1);
+    expect(process.exitCode, 0);


That's not right. If a lint if emitted, it should emit 1 not 0.

I'm considering the following configuration.

ErrorSeverity.info => Optional error (default exit code 0)

ErrorSeverity.warning => Default error, but optionally skippable (default exit code 1)

ErrorSeverity.error => Always error (default exit code 1)

This is consistent with the terminology used in Dart analyze.

modifying the parameter definition here. So it will be changed to return an exit code of 0.

Could you please share your thoughts on the design for this ErrorSeverity? I would appreciate hearing your considerations and insights.

I have contemplated aligning it with the fundamental design of Dart analyzer.
However, I am unsure if it aligns with the overall philosophy of this package.

I would prefer basing custom_lint on flutter analyze rather than dart analyze.

With flutter analyze, infos lead to an exitCode 1 too, and you do flutter analyze --no-fatal-infos

rrousselGit · 2023-04-19T12:08:41Z

packages/custom_lint/test/cli_test.dart

  )
 ]);

 final helloWordPluginSource = createPluginSource([
  TestLintRule(
    code: 'hello_world',
    message: 'Hello world',
+    errorSeverity: 'ErrorSeverity.WARNING',


This impacts existing tests.

You should instead make a separate variable with the warning flag instead of reusing something already used by different tests but with the info status.

before considering setting up another test, I would like to confirm this discussion first.

rrousselGit · 2023-04-19T12:09:21Z

packages/custom_lint/test/cli_test.dart

@@ -258,27 +334,27 @@ class _Lint extends DartLintRule {
  ) {
    final line2 = resolver.lineInfo.getOffsetOfLine(1);
    reporter.reportErrorForOffset(
-      const LintCode(name: 'x2', problemMessage: 'x2'),
+      const LintCode(name: 'x2', problemMessage: 'x2', errorSeverity: ErrorSeverity.WARNING),


Why these changes?

The reason is the same as here.

rrousselGit · 2023-04-19T12:10:06Z

packages/custom_lint/test/create_project.dart

@@ -37,6 +38,7 @@ class TestLintRule {
  final String onVariable;
  final String ruleMembers;
  final List<TestLintFix> fixes;
+  final String errorSeverity;


Don't use a String for this. Either make an enum or use the official ErrorSeverity object.

fixed commit 8fffd8d

rrousselGit · 2023-04-19T12:11:44Z

Hello! Sorry for the slow review. The notification was lost in the sea of notifications I receive :(

I had another pass at this. It's looking mostly good! But there are some relatively minor issues. Mind fixing them?

yamarkz · 2023-04-19T13:58:21Z

@rrousselGit

Thank you for maintaining the package, No problem.

I will follow your path.

rrousselGit · 2023-06-08T10:17:07Z

👋 Do you still plan to work on this? :)

yamarkz · 2023-06-09T01:44:49Z

@rrousselGit

Oops I may have misunderstood. I expected to be presented with a completely different alternative.
Can I take ownership of PR? if yes. Can I proceed with the revision according to the review comments?

Our development team is still waiting for this change.

rrousselGit · 2023-06-09T13:12:37Z

👋

I don't quite understand what the confusion is. Do you mind explaining?
But anyway, this PR is waiting for you to fix the various points I raised previously. Once they are fixed and I think all changes look good, this can be merged

Co-authored-by: Remi Rousselet <darky12s@gmail.com>

rrousselGit · 2023-08-14T08:52:16Z

packages/custom_lint/test/cli_test.dart

+        await cli.entrypoint(['--no-fatal-infos']);
+
+        expect(err, emitsDone);
+        expect(exitCode, 0);


Could you test that out still logs the infos/warnings?

fixed commit 882b9bb

rrousselGit

The code is looking good!

If you update the tests to check that out still includes the logs, I'm happy to merge this asap and deploy it :)

rrousselGit · 2023-08-29T06:08:15Z

Great thanks! I'll wait for the CI to complete and merge this

packages/custom_lint/test/goldens/server_test/redirect_logs.golden

yamarkz added 2 commits March 22, 2023 22:53

Support error severity screening option

0ba210f

Add error severity screening option test

3a8107d

vercel bot deployed to Preview March 22, 2023 14:27 View deployment

yamarkz added 2 commits April 8, 2023 17:51

fix merge conflict

559ee59

fix test case

b759a52

vercel bot deployed to Preview April 8, 2023 08:56 View deployment

sort arguments and remove default value

0e4cbbe

vercel bot deployed to Preview April 8, 2023 09:00 View deployment

rrousselGit reviewed Apr 19, 2023

View reviewed changes

packages/custom_lint/lib/custom_lint.dart Outdated Show resolved Hide resolved

rrousselGit reviewed Apr 19, 2023

View reviewed changes

packages/custom_lint/lib/custom_lint.dart Outdated Show resolved Hide resolved

rrousselGit reviewed Apr 19, 2023

View reviewed changes

rrousselGit self-assigned this May 24, 2023

Merge branch 'main' into support-error-severity-screening-option

471053b

vercel bot deployed to Preview June 18, 2023 00:53 View deployment

Update packages/custom_lint/lib/custom_lint.dart

9aeebb7

Co-authored-by: Remi Rousselet <darky12s@gmail.com>

yamarkz added 3 commits August 12, 2023 11:13

Rewrite conditional branch

c309f2a

Set default option true fatal-infos

8549d02

Fix written test

16e1a6a

vercel bot deployed to Preview August 12, 2023 02:37 View deployment

Remove unnecessary test code

bf8282b

vercel bot deployed to Preview August 12, 2023 02:40 View deployment

Add no option test code

8522417

vercel bot deployed to Preview August 12, 2023 03:09 View deployment

Fix test case name

0267564

vercel bot deployed to Preview August 12, 2023 03:11 View deployment

yamarkz requested a review from rrousselGit August 12, 2023 03:30

rrousselGit reviewed Aug 14, 2023

View reviewed changes

rrousselGit approved these changes Aug 14, 2023

View reviewed changes

Rewrite test case log output text

882b9bb

vercel bot deployed to Preview August 14, 2023 09:51 View deployment

yamarkz requested a review from rrousselGit August 14, 2023 09:57

Update redirect_logs.golden

1717a10

vercel bot deployed to Preview August 29, 2023 06:23 View deployment

Update redirect_logs.golden

440f858

vercel bot deployed to Preview August 29, 2023 06:43 View deployment

rrousselGit reviewed Aug 29, 2023

View reviewed changes

packages/custom_lint/test/goldens/server_test/redirect_logs.golden Outdated Show resolved Hide resolved

Apply suggestions from code review

63badeb

vercel bot deployed to Preview August 29, 2023 06:44 View deployment

Fix golden

139fa68

vercel bot deployed to Preview August 29, 2023 06:46 View deployment

Increase timeout

c03ffd4

vercel bot deployed to Preview August 29, 2023 07:34 View deployment

rrousselGit merged commit f9c988d into invertase:main Aug 29, 2023

oskar-zeinomahmalat-sonarsource pushed a commit to oskar-zeinomahmalat-sonarsource/dart_custom_lint that referenced this pull request Dec 3, 2024

Support error severity screening option (invertase#129)

1b83649

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support error severity screening option #129

Support error severity screening option #129

yamarkz commented Mar 22, 2023

docs-page bot commented Mar 22, 2023

vercel bot commented Mar 22, 2023 •

edited

Loading

CLAassistant commented Mar 22, 2023 •

edited

Loading

yamarkz commented Mar 22, 2023

yamarkz commented Apr 8, 2023

rrousselGit Apr 19, 2023

yamarkz Jun 18, 2023

rrousselGit Apr 19, 2023

yamarkz Jun 18, 2023 •

edited

Loading

rrousselGit Apr 19, 2023

yamarkz Jun 18, 2023

rrousselGit Jun 21, 2023

rrousselGit Apr 19, 2023

yamarkz Jun 18, 2023

rrousselGit Apr 19, 2023

yamarkz Jun 18, 2023

rrousselGit Apr 19, 2023

yamarkz Jun 18, 2023

rrousselGit commented Apr 19, 2023

yamarkz commented Apr 19, 2023

rrousselGit commented Jun 8, 2023

yamarkz commented Jun 9, 2023 •

edited

Loading

rrousselGit commented Jun 9, 2023

rrousselGit Aug 14, 2023

yamarkz Aug 14, 2023

rrousselGit left a comment

rrousselGit commented Aug 29, 2023

Support error severity screening option #129

Support error severity screening option #129

Conversation

yamarkz commented Mar 22, 2023

docs-page bot commented Mar 22, 2023

vercel bot commented Mar 22, 2023 • edited Loading

CLAassistant commented Mar 22, 2023 • edited Loading

yamarkz commented Mar 22, 2023

yamarkz commented Apr 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamarkz Jun 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrousselGit commented Apr 19, 2023

yamarkz commented Apr 19, 2023

rrousselGit commented Jun 8, 2023

yamarkz commented Jun 9, 2023 • edited Loading

rrousselGit commented Jun 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrousselGit left a comment

Choose a reason for hiding this comment

rrousselGit commented Aug 29, 2023

vercel bot commented Mar 22, 2023 •

edited

Loading

CLAassistant commented Mar 22, 2023 •

edited

Loading

yamarkz Jun 18, 2023 •

edited

Loading

yamarkz commented Jun 9, 2023 •

edited

Loading