Pass all linted file names to formatter #3838

mastermatt · 2018-04-17T06:18:54Z

PR checklist

Addresses an existing issue: Checkstyle formatter does not include error free files. #3837
Enhancement
- Includes tests
Documentation update

Overview of change:

Allows the Checkstyle and JUnit formatters to output the
files that did not have any failures.

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

Adding a third argument to IFormatter.format seems like a more reasonable option than changing its signature and having a breaking change, but it's not pretty.

CHANGELOG.md entry:

[enhancement] Formatters now receive the full list of of linted file paths as a third argument.
[enhancement] Checkstyle formatter includes every file linted regardless of lint errors.

palantirtech · 2018-04-17T06:18:55Z

Thanks for your interest in palantir/tslint, @mastermatt! 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.

giladgray

this looks really great! just one small comment about the formatter interface

giladgray · 2018-05-07T17:49:58Z

src/language/formatter/abstractFormatter.ts

@@ -20,7 +20,7 @@ import { IFormatter, IFormatterMetadata } from "./formatter";

 export abstract class AbstractFormatter implements IFormatter {
    public static metadata: IFormatterMetadata;
-    public abstract format(failures: RuleFailure[]): string;
+    public abstract format(failures: RuleFailure[], fixes?: RuleFailure[], fileNames?: string[]): string;


correct me if i'm wrong: these parameters in the interface definition should not have ? as they are always passed to the implementation. implementors should instead choose to omit the args from their implementations if unused.

@giladgray I think you're right. I was following the convention already on fixes in IFormatter, however, since the args are always provided they shouldn't be marked as optional.
I've pushed a patch to remove the ?s.

giladgray · 2018-05-07T17:50:31Z

src/language/formatter/formatter.ts

     */
-    format(failures: RuleFailure[], fixes?: RuleFailure[]): string;
+    format(failures: RuleFailure[], fixes?: RuleFailure[], fileNames?: string[]): string;


again, pretty sure the params should not be marked optional

not sure what you mean or if this is still relevant. required arguments can't come after optional arguments. and we don't want to break this API.

As noted in comments on palantir#3838, these parameters in the interface definition should not have `?` as they are always passed to the implementation. Implementors should instead choose to _omit_ the args from their implementations if unused.

mastermatt · 2018-05-07T18:43:24Z

@giladgray I was quick to push a patch that removed the ?s, but now looking at the CI errors I remember why I had kept them.
The formatters are meant to be extended by third parties. As illustrated by the errors when trying to build, no longer making them optional breaks backwards compatibility with existing formatters and therefore this change would require a major release.
I'm going to remove the second commit and leave this PR as it was.

giladgray · 2018-05-07T19:10:57Z

@mastermatt the CI failures do not look related to the formatter syntax. i don't see any such incompatibilities.

mastermatt · 2018-05-07T19:41:23Z

@giladgray these are the errors I get:

> ~/src/tslint: yarn compile
yarn run v1.6.0
$ npm-run-all -p compile:core compile:test -s compile:scripts
$ tsc -p test
$ tsc -p src
test/formatters/codeFrameFormatterTests.ts(95,39): error TS2554: Expected 3 arguments, but got 1.
test/formatters/codeFrameFormatterTests.ts(101,24): error TS2554: Expected 3 arguments, but got 1.
test/formatters/externalFormatterTests.ts(49,30): error TS2554: Expected 3 arguments, but got 1.
test/formatters/externalFormatterTests.ts(58,24): error TS2554: Expected 3 arguments, but got 1.
test/formatters/fileslistFormatterTests.ts(44,30): error TS2554: Expected 3 arguments, but got 1.
test/formatters/fileslistFormatterTests.ts(49,24): error TS2554: Expected 3 arguments, but got 1.
test/formatters/jsonFormatterTests.ts(104,41): error TS2554: Expected 3 arguments, but got 1.
test/formatters/jsonFormatterTests.ts(109,35): error TS2554: Expected 3 arguments, but got 1.
test/formatters/junitFormatterTests.ts(78,22): error TS2554: Expected 3 arguments, but got 1.
test/formatters/junitFormatterTests.ts(82,24): error TS2554: Expected 3 arguments, but got 1.
test/formatters/msbuildFormatterTests.ts(49,30): error TS2554: Expected 3 arguments, but got 1.
test/formatters/msbuildFormatterTests.ts(54,24): error TS2554: Expected 3 arguments, but got 1.
test/formatters/pmdFormatterTests.ts(63,22): error TS2554: Expected 3 arguments, but got 1.
test/formatters/pmdFormatterTests.ts(67,24): error TS2554: Expected 3 arguments, but got 1.
test/formatters/proseFormatterTests.ts(49,30): error TS2554: Expected 3 arguments, but got 1.
test/formatters/proseFormatterTests.ts(72,30): error TS2554: Expected 3 arguments, but got 2.
test/formatters/proseFormatterTests.ts(77,24): error TS2554: Expected 3 arguments, but got 1.
test/formatters/stylishFormatterTests.ts(57,22): error TS2554: Expected 3 arguments, but got 1.
test/formatters/stylishFormatterTests.ts(61,24): error TS2554: Expected 3 arguments, but got 1.
test/formatters/tapFormatterTests.ts(50,30): error TS2554: Expected 3 arguments, but got 1.
test/formatters/tapFormatterTests.ts(55,24): error TS2554: Expected 3 arguments, but got 1.
test/formatters/verboseFormatterTests.ts(49,30): error TS2554: Expected 3 arguments, but got 1.
test/formatters/verboseFormatterTests.ts(54,24): error TS2554: Expected 3 arguments, but got 1.
test/formatters/vsoFormatterTests.ts(48,30): error TS2554: Expected 3 arguments, but got 1.
test/formatters/vsoFormatterTests.ts(68,30): error TS2554: Expected 3 arguments, but got 2.
test/formatters/vsoFormatterTests.ts(73,24): error TS2554: Expected 3 arguments, but got 1.```

mastermatt · 2018-06-06T21:07:57Z

@giladgray any thoughts on this?

johnwiseheart · 2018-07-19T16:09:44Z

@mastermatt as a brief follow up, it looks to me like the failing tests are because those tests are using the format method calling it with only two arguments. By adding another argument, you probably need to update those tests.

mastermatt · 2018-10-26T19:05:26Z

Could I get some eyes on this again?

To address the comments by @giladgray and @johnwiseheart: @giladgray was right with his first set of comments. The args should not be marked as optional on the AbstractFormatter class and allow implementers to optionally omit those args if they choose. However, making those same two args required in IFormatter is problematic as it would cause the tests of third-party formatters to suddenly fail.
I propose moving forward with this PR as is, since marking fixes in IFormatter.format as optional is the status quo, and I'll open another PR that fixes IFormatter. Since the new PR will have back-breaking changes it can sit around until the next major is ready to go.

giladgray · 2018-11-29T18:40:56Z

@mastermatt can you resolve merge conflicts here so we can merge this PR?

For palantir#3837, allows the Checkstyle and JUnit formatters to output the files that did not have any failures. Adding a third argument to `IFormatter.format` seems like a more reasonable option than changing its signature and having a breaking change.

mastermatt · 2018-11-30T00:04:24Z

@giladgray done.

adidahiya · 2019-01-09T01:49:20Z

src/formatters/checkstyleFormatter.ts

+        const groupedFailures: { [k: string]: RuleFailure[] } = {};
+        for (const failure of failures) {
+            const fileName = failure.getFileName();
+            if (fileName in groupedFailures) {


typically we don't use the in operator (sorry, I know we should lint for it)

adidahiya · 2019-01-09T01:50:48Z

src/language/formatter/formatter.ts

     */
-    format(failures: RuleFailure[], fixes?: RuleFailure[]): string;
+    format(failures: RuleFailure[], fixes?: RuleFailure[], fileNames?: string[]): string;


not sure what you mean or if this is still relevant. required arguments can't come after optional arguments. and we don't want to break this API.

adidahiya · 2019-01-09T01:51:12Z

src/language/formatter/formatter.ts

@@ -55,6 +55,7 @@ export interface IFormatter {
     * Formats linter results
     * @param failures Linter failures that were not fixed
     * @param fixes Fixed linter failures. Available when the `--fix` argument is used on the command line
+     * @param fileNames All of file paths that were linted


"All of the file paths that were linted"

adidahiya

LGTM, but let's wait until we're ready to release 5.13 before merging this (I think there will be a 5.12.1 release before that)

adidahiya · 2019-02-23T20:56:54Z

Lint failure is unrelated due to a semantic merge conflict, I'll fix it on master

mastermatt mentioned this pull request Apr 24, 2018

Junit formatter prevents Jenkins picking up results on success #3744

Closed

giladgray reviewed May 7, 2018

View reviewed changes

mastermatt force-pushed the file-names-to-formatters branch from ba646a1 to 76c2ece Compare May 7, 2018 18:44

mastermatt force-pushed the file-names-to-formatters branch from 76c2ece to 1690abb Compare May 7, 2018 21:56

mastermatt force-pushed the file-names-to-formatters branch 2 times, most recently from ad2e789 to 7a6fbe8 Compare October 26, 2018 18:36

JoshuaKGoldberg approved these changes Nov 7, 2018

View reviewed changes

JoshuaKGoldberg added PR: waiting for reviewer PR: merge target branch labels Nov 7, 2018

ajafff mentioned this pull request Nov 9, 2018

bifrost: pass all file names to formatters fimbullinter/wotan#456

Closed

mastermatt force-pushed the file-names-to-formatters branch from 7a6fbe8 to 013a9ee Compare November 30, 2018 00:01

adidahiya reviewed Jan 9, 2019

View reviewed changes

adidahiya added 2 commits January 8, 2019 20:52

Update checkstyleFormatter.ts

cc53fd9

Update formatter.ts

afe22b5

adidahiya approved these changes Jan 9, 2019

View reviewed changes

adidahiya removed PR: merge target branch PR: waiting for reviewer labels Jan 9, 2019

adidahiya added this to the 5.13.0 milestone Jan 17, 2019

adidahiya self-assigned this Jan 17, 2019

Merge branch 'master' into file-names-to-formatters

ff5bf85

adidahiya changed the title ~~Pass all listed file names to formatter.~~ Pass all linted file names to formatter Feb 23, 2019

adidahiya merged commit 9000479 into palantir:master Feb 23, 2019

mastermatt deleted the file-names-to-formatters branch February 25, 2019 16:58

mastermatt mentioned this pull request Feb 25, 2019

Checkstyle formatter does not include error free files. #3837

Closed

snyk-bot mentioned this pull request Oct 11, 2019

[Snyk] Upgrade tslint from 5.11.0 to 5.20.0 Narshe1412/nuarca-sql#21

Merged

snyk-bot mentioned this pull request Oct 26, 2019

[Snyk] Upgrade tslint from 5.12.1 to 5.20.0 ajesse11x/caprover#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass all linted file names to formatter #3838

Pass all linted file names to formatter #3838

mastermatt commented Apr 17, 2018

palantirtech commented Apr 17, 2018

giladgray left a comment

giladgray May 7, 2018

mastermatt May 7, 2018

giladgray May 7, 2018

adidahiya Jan 9, 2019

mastermatt commented May 7, 2018

giladgray commented May 7, 2018

mastermatt commented May 7, 2018

mastermatt commented Jun 6, 2018

johnwiseheart commented Jul 19, 2018

mastermatt commented Oct 26, 2018

giladgray commented Nov 29, 2018

mastermatt commented Nov 30, 2018

adidahiya Jan 9, 2019

adidahiya Jan 9, 2019

adidahiya Jan 9, 2019

adidahiya left a comment

adidahiya commented Feb 23, 2019

Pass all linted file names to formatter #3838

Pass all linted file names to formatter #3838

Conversation

mastermatt commented Apr 17, 2018

PR checklist

Overview of change:

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

CHANGELOG.md entry:

palantirtech commented Apr 17, 2018

giladgray left a comment

Choose a reason for hiding this comment

giladgray May 7, 2018

Choose a reason for hiding this comment

mastermatt May 7, 2018

Choose a reason for hiding this comment

giladgray May 7, 2018

Choose a reason for hiding this comment

adidahiya Jan 9, 2019

Choose a reason for hiding this comment

mastermatt commented May 7, 2018

giladgray commented May 7, 2018

mastermatt commented May 7, 2018

mastermatt commented Jun 6, 2018

johnwiseheart commented Jul 19, 2018

mastermatt commented Oct 26, 2018

giladgray commented Nov 29, 2018

mastermatt commented Nov 30, 2018

adidahiya Jan 9, 2019

Choose a reason for hiding this comment

adidahiya Jan 9, 2019

Choose a reason for hiding this comment

adidahiya Jan 9, 2019

Choose a reason for hiding this comment

adidahiya left a comment

Choose a reason for hiding this comment

adidahiya commented Feb 23, 2019