Sort failures by failure's line and character #3345

eggggger · 2017-10-19T07:53:24Z

PR checklist

Addresses an existing issue: #3335
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

Is there anything you'd like reviewers to focus on?

CHANGELOG.md entry:

[enhancement] Sort failures by line and character for formatters

palantirtech · 2017-10-19T07:53:26Z

Thanks for your interest in palantir/tslint, @eggggger! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

ajafff · 2017-10-19T15:31:13Z

src/linter.ts

-        const output = formatter.format(this.failures, this.fixes);
+        // Sort failures by failure's line and character.
+        const failures = this.sortFailures(this.failures);
+        const fixes = this.sortFailures(this.fixes);


the fixes are actually never really used by a formatter, so they don't need to be sorted.

ajafff · 2017-10-19T15:34:17Z

src/linter.ts

@@ -131,19 +131,40 @@ class Linter {
            throw new Error(`formatter '${formatterName}' not found`);
        }

-        const output = formatter.format(this.failures, this.fixes);
+        // Sort failures by failure's line and character.
+        const failures = this.sortFailures(this.failures);


I'd suggest to move the sorting of failures to each formatter that needs it. Formatters for machine consumption shouldn't rely on any order.
That means there are 4 formatters affected: prose, verbose, stylish and code-frame.

ajafff · 2017-10-19T15:35:21Z

src/linter.ts

+                if (failureALineAndCharacter.line === failureBLineAndCharacter.line) {
+                    return failureALineAndCharacter.character - failureBLineAndCharacter.character;
+                }
+                return failureALineAndCharacter.line - failureBLineAndCharacter.line;


instead of comparing line and character, you can simply compare the position

ajafff · 2017-10-19T15:37:38Z

src/linter.ts

+        });
+
+        return sortedFailures;
+    }


As described above, I'd like to move this to the formatters. To make the compare logic reusable, consider adding it as static method to class RuleFailure in src/language/rule/rule.ts:

public static compare(a: RuleFailure, b: RuleFailure): number { if (a.fileName !== b.fileName) { return a.fileName < b.fileName ? -1 : 1; } return a.startPosition.getPosition() - b.startPosition.getPosition(); }

eggggger · 2017-10-19T17:26:34Z

@ajafff Thank you for your suggestion，and is it better to move sorting to AbstractFormatter?

ajafff · 2017-10-19T17:33:45Z

src/formatters/codeFrameFormatter.ts

@@ -97,4 +98,8 @@ export class Formatter extends AbstractFormatter {

        return `${outputLines.join("\n")}\n`;
    }
+
+    public sortFailures(failures: RuleFailure[]): RuleFailure[] {


these methods don't need to be public

ajafff · 2017-10-19T17:38:38Z

src/formatters/codeFrameFormatter.ts

@@ -97,4 +98,8 @@ export class Formatter extends AbstractFormatter {

        return `${outputLines.join("\n")}\n`;
    }
+
+    public sortFailures(failures: RuleFailure[]): RuleFailure[] {
+        return failures.slice().sort(RuleFailure.compare);


Personally, I would not write a method for this simple statement. If you want to keep the method, it makes sense to move it to AbstractFormatter as you already noted. Works for me either way.

ajafff · 2017-10-19T18:11:29Z

@eggggger You're almost done. CI fails due to linting errors:

/home/ubuntu/tslint/src/formatters/stylishFormatter.ts
ERROR: 42:47  semicolon        Missing semicolon

/home/ubuntu/tslint/src/language/rule/rule.ts
ERROR: 261:5  member-ordering  Declaration of public static method not allowed after declaration of public instance method. Instead, this should come after private instance fields.

you can also run linting locally with yarn lint

ajafff · 2017-10-20T07:29:42Z

Thanks @eggggger

eggggger force-pushed the sort-failures branch from 7351684 to f16d4d3 Compare October 19, 2017 07:54

eggggger changed the title ~~Sort failures by failure\'s line and character~~ Sort failures by failure's line and character Oct 19, 2017

Sort failures by failure's line and character

094f807

eggggger force-pushed the sort-failures branch from f16d4d3 to 094f807 Compare October 19, 2017 14:20

ajafff suggested changes Oct 19, 2017

View reviewed changes

Move sorting of failures to human-readable formatter

0bdd50e

ajafff reviewed Oct 19, 2017

View reviewed changes

Move sorting to AbstractFormatter & fix tests

72e1f13

ajafff approved these changes Oct 19, 2017

View reviewed changes

Fix lint

399dc6f

ajafff merged commit f1e0de2 into palantir:master Oct 20, 2017

ajafff mentioned this pull request Oct 20, 2017

Sort fileFailures by line and character? #3335

Closed

HyphnKnight pushed a commit to HyphnKnight/tslint that referenced this pull request Apr 9, 2018

Sort failures by failure's line and character (palantir#3345)

d87100d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort failures by failure's line and character #3345

Sort failures by failure's line and character #3345

eggggger commented Oct 19, 2017

palantirtech commented Oct 19, 2017

ajafff Oct 19, 2017

ajafff Oct 19, 2017

ajafff Oct 19, 2017

ajafff Oct 19, 2017

eggggger commented Oct 19, 2017 •

edited

ajafff Oct 19, 2017

ajafff Oct 19, 2017

ajafff commented Oct 19, 2017

ajafff commented Oct 20, 2017

Sort failures by failure's line and character #3345

Sort failures by failure's line and character #3345

Conversation

eggggger commented Oct 19, 2017

PR checklist

Overview of change:

Is there anything you'd like reviewers to focus on?

CHANGELOG.md entry:

palantirtech commented Oct 19, 2017

ajafff Oct 19, 2017

Choose a reason for hiding this comment

ajafff Oct 19, 2017

Choose a reason for hiding this comment

ajafff Oct 19, 2017

Choose a reason for hiding this comment

ajafff Oct 19, 2017

Choose a reason for hiding this comment

eggggger commented Oct 19, 2017 • edited

ajafff Oct 19, 2017

Choose a reason for hiding this comment

ajafff Oct 19, 2017

Choose a reason for hiding this comment

ajafff commented Oct 19, 2017

ajafff commented Oct 20, 2017

eggggger commented Oct 19, 2017 •

edited