Support default/json output formats #86

kuhnroyal · 2023-02-21T17:50:04Z

This is an initial start in order to eventually support invertase/github-action-dart-analyzer#199

It works pretty much, most of the output code comes from the Dart SDK. Wonder if there is a way to use it directly?

Fixes #22
Fixes #101

vercel · 2023-02-21T17:50:07Z

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

Name	Status	Preview	Comments	Updated
custom-lint-website	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 27, 2023 at 8:31PM (UTC)

rrousselGit · 2023-02-23T09:26:44Z

Wonder if there is. a way to use it directly?

I don't know honestly.

Problem is currently the print overrides which don't work well together with cli_util package and the output expectations in the tests.

What's the issue exactly?

kuhnroyal · 2023-02-23T10:21:01Z

What's the issue exactly?

The cli_util package which the Dart analyzer also uses, has a Logger which uses print internally: https://github.com/dart-lang/cli_util/blame/master/lib/cli_logging.dart#L140
But there is a Zone override to write the log file, maybe the log file should only be generated for the server and not for the client?

rrousselGit · 2023-02-27T18:14:37Z

No it is crutial that logs in the client are reported. That's probably the most important bit.

But I do not understand why you need to disable log reporting in the client for this

rrousselGit · 2023-02-27T18:15:15Z

Maybe you're looking at modifying the CommandCustomLintDelegate?

kuhnroyal · 2023-02-27T18:45:02Z

Well cli_util uses print and in the current setup print goes into the log file and not to stdout as far as I can tell :)
Can we use some dedicated logging for the log file instead of the zone override?

kuhnroyal · 2023-02-27T19:08:53Z

Maybe I am also just stupid. I fixed the tests and added screenshots of the sample outputs.

rrousselGit · 2023-02-27T19:25:27Z

Using zone overrides is expected. It's the most user-friendly experience, as they can just use "print". That's natural
Your implementation should support it.

As I said, you should be able to change CommandCustomLintDelegate to your liking.

kuhnroyal · 2023-02-27T20:41:25Z

I think this is good now, I added a test for the json format and extended the sorting test to take the severity into account.

praxder · 2023-03-03T20:15:49Z

I'm looking forward to this being merged. :)

rrousselGit · 2023-03-08T18:45:06Z

packages/custom_lint/lib/custom_lint.dart

@@ -70,58 +75,26 @@ Future<void> _runPlugins(
  CustomLintRunner runner, {
  required bool reload,
 }) async {
+  final log = Logger.standard();
+
+  log.progress('Analyzing');


The progress is never stopped

cli_util automatically stops progress if any other output is performed.

packages/custom_lint/test/cli_test.dart

rrousselGit · 2023-03-08T18:46:29Z

packages/custom_lint/lib/custom_lint.dart

@@ -33,6 +33,7 @@ q: Quit
 /// Watch mode cannot be enabled if in release mode.
 Future<void> customLint({
  bool watchMode = true,
+  String format = 'default',


Don't use Strings here. Use an enum

Yea, will do.

rrousselGit · 2023-03-08T18:49:35Z

packages/custom_lint/lib/src/output.dart

+import 'package:path/path.dart' as p;
+
+/// The maximum length of any of the existing severity labels.
+const int _severityWidth = 7;


It's weird to have this hardcoded I think. We should be able to easily compute this dymically

Yea maybe, but I wanted to keep the whole output code as close to the original Dart analyzer code as possible. This way custom_lint has the same consistent output formatting and it is easier to keep up with changes in the Dart analyzer.

rrousselGit · 2023-03-08T18:50:17Z

packages/custom_lint/lib/src/output.dart

+/// is used as the usageLineLength.
+int? get _dartdevUsageLineLength =>
+    stdout.hasTerminal ? stdout.terminalColumns : null;


I don't understand how the function name and its implementation are related

Just taken from the Dart analyzer - it is the max line length in a terminal before a line break happens, but only if stdout is connected a terminal.

rrousselGit · 2023-03-08T18:53:23Z

packages/custom_lint/lib/src/output.dart

+      for (final contextMessage in error.contextMessages!) {
+        final startOffset = contextMessage.location.offset;
+        contextMessages.add({
+          'location': location(


All of this serialization logic sounds redundant with the AnalyzerConverter class. Which allows converting an AnalysisError from analyzer into one from analyzer_plugin. And the latter has a toJson

Same as above, this is just taken from Dart analyzer and is different from the toJson output I believe.

rrousselGit · 2023-03-08T18:54:33Z

packages/custom_lint/lib/src/output.dart

+
+/// Emits a list of lints in the Dart analyzer style default format.
+@internal
+void emitDefaultFormat(


Please don't change the output of the default mode in this PR.
That makes reviewing this PR a bit difficult

If we want to improve the rendering, this should be a separate issue/PR

Since this is apparently taken from the analyzer, it'd be great to document that too. Maybe have all the analyzer code in a separate file with a link to their original source

Yea that is basically most of the output file, I will try to split the custom_lint parts out and add a license-info/documentation at the top.

But splitting up the json/default format into 2 PRs is kinda hard because they are pretty interwoven until the last print step.

Sounds good then. I'm fine with having one PR if you move analyzer-stuff in its own file :)
Otherwise it's a bit hard to know what you wrote vs what's forked

kuhnroyal · 2023-03-09T13:37:37Z

Are the 2 output examples in the screenshots something that we generally agree on?

rrousselGit · 2023-03-09T13:55:56Z

packages/custom_lint/test/cli_test.dart

@@ -21,6 +22,8 @@ final helloWordPluginSource = createPluginSource([
  )
 ]);

+final ansi = Ansi(Ansi.terminalSupportsAnsi);


The tests shouldn't rely on terminalSupportsAnsi
Instead they should use a consistent on/off status.

We'd want to test the output both with and without terminalSupportsAnsi then.
Which also means a test which doesn't strip ansi codes to make sure they are present if you don't mind.

Will see if we can simulate this somehow but Github runner terminals don't support ansi.

You don't need github runners for this.

rrousselGit · 2023-03-14T08:48:56Z

If you don't mind, I'll wait until I can fix the funky platform crash before merging this.

kuhnroyal · 2023-03-17T09:43:28Z

I am gonna split this up in smaller parts.

) * Fix false-positives for cyclomatic complexity metric lints * fmt --------- Co-authored-by: Yurii Prykhodko <yurii.prykhodko@solid.software>

vercel bot deployed to Preview February 21, 2023 17:50 View deployment

Support default/json output formats

8ac6658

kuhnroyal force-pushed the output-format branch from 89f75ab to 52a6562 Compare February 27, 2023 19:06

vercel bot deployed to Preview February 27, 2023 19:06 View deployment

Fix tests & formatting

40e2eac

kuhnroyal force-pushed the output-format branch from 52a6562 to 40e2eac Compare February 27, 2023 19:13

vercel bot deployed to Preview February 27, 2023 19:13 View deployment

Fix cli test in non-ansi terminal

90658d0

vercel bot deployed to Preview February 27, 2023 19:55 View deployment

kuhnroyal added 2 commits February 27, 2023 21:30

Extend sorting test for severity

0daa7b4

Add test for json format

98cf097

vercel bot deployed to Preview February 27, 2023 20:31 View deployment

kuhnroyal marked this pull request as ready for review February 27, 2023 20:41

kuhnroyal mentioned this pull request Feb 27, 2023

Added severity to CLI output #48

Merged

rrousselGit reviewed Mar 8, 2023

View reviewed changes

packages/custom_lint/test/cli_test.dart Show resolved Hide resolved

rrousselGit reviewed Mar 8, 2023

View reviewed changes

rrousselGit reviewed Mar 9, 2023

View reviewed changes

kuhnroyal closed this Mar 17, 2023

yamarkz mentioned this pull request Mar 19, 2023

feat: improve output formatting when running run custom_lint #101

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support default/json output formats #86

Support default/json output formats #86

kuhnroyal commented Feb 21, 2023 •

edited

vercel bot commented Feb 21, 2023 •

edited

rrousselGit commented Feb 23, 2023

kuhnroyal commented Feb 23, 2023

rrousselGit commented Feb 27, 2023

rrousselGit commented Feb 27, 2023

kuhnroyal commented Feb 27, 2023

kuhnroyal commented Feb 27, 2023

rrousselGit commented Feb 27, 2023

kuhnroyal commented Feb 27, 2023

praxder commented Mar 3, 2023

rrousselGit Mar 8, 2023

kuhnroyal Mar 9, 2023

rrousselGit Mar 8, 2023

kuhnroyal Mar 9, 2023

rrousselGit Mar 8, 2023

kuhnroyal Mar 9, 2023

rrousselGit Mar 8, 2023

kuhnroyal Mar 9, 2023

rrousselGit Mar 8, 2023

kuhnroyal Mar 9, 2023

rrousselGit Mar 8, 2023

rrousselGit Mar 9, 2023

kuhnroyal Mar 9, 2023

kuhnroyal Mar 9, 2023

rrousselGit Mar 9, 2023

kuhnroyal commented Mar 9, 2023

rrousselGit Mar 9, 2023

kuhnroyal Mar 9, 2023

rrousselGit Mar 14, 2023

rrousselGit commented Mar 14, 2023

kuhnroyal commented Mar 17, 2023

Support default/json output formats #86

Support default/json output formats #86

Conversation

kuhnroyal commented Feb 21, 2023 • edited

vercel bot commented Feb 21, 2023 • edited

rrousselGit commented Feb 23, 2023

kuhnroyal commented Feb 23, 2023

rrousselGit commented Feb 27, 2023

rrousselGit commented Feb 27, 2023

kuhnroyal commented Feb 27, 2023

kuhnroyal commented Feb 27, 2023

rrousselGit commented Feb 27, 2023

kuhnroyal commented Feb 27, 2023

praxder commented Mar 3, 2023

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

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

kuhnroyal commented Mar 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrousselGit commented Mar 14, 2023

kuhnroyal commented Mar 17, 2023

kuhnroyal commented Feb 21, 2023 •

edited

vercel bot commented Feb 21, 2023 •

edited