Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Pass all linted file names to formatter #3838

Merged
merged 4 commits into from
Feb 23, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 30 additions & 31 deletions src/formatters/checkstyleFormatter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export class Formatter extends AbstractFormatter {
formatterName: "checkstyle",
description: "Formats errors as though they were Checkstyle output.",
descriptionDetails: Utils.dedent`
Imitates the XMLLogger from Checkstyle 4.3. All failures have the 'warning' severity.`,
Imitates the XMLLogger from Checkstyle 4.3. All failures have the 'warning' severity. Files without errors are still included.`,
sample: Utils.dedent`
<?xml version="1.0" encoding="utf-8"?>
<checkstyle version="4.3">
Expand All @@ -38,39 +38,38 @@ export class Formatter extends AbstractFormatter {
};
/* tslint:enable:object-literal-sort-keys */

public format(failures: RuleFailure[]): string {
let output = '<?xml version="1.0" encoding="utf-8"?><checkstyle version="4.3">';

if (failures.length !== 0) {
const failuresSorted = failures.sort((a, b) =>
a.getFileName().localeCompare(b.getFileName()),
);
let previousFilename: string | null = null;
for (const failure of failuresSorted) {
const severity = failure.getRuleSeverity();
if (failure.getFileName() !== previousFilename) {
if (previousFilename !== null) {
output += "</file>";
}
previousFilename = failure.getFileName();
output += `<file name="${this.escapeXml(failure.getFileName())}">`;
}
output += `<error line="${failure.getStartPosition().getLineAndCharacter().line +
1}" `;
output += `column="${failure.getStartPosition().getLineAndCharacter().character +
1}" `;
output += `severity="${severity}" `;
output += `message="${this.escapeXml(failure.getFailure())}" `;
// checkstyle parser wants "source" to have structure like <anything>dot<category>dot<type>
output += `source="failure.tslint.${this.escapeXml(failure.getRuleName())}" />`;
}
if (previousFilename !== null) {
output += "</file>";
public format(failures: RuleFailure[], _fixes: RuleFailure[], fileNames: string[]): string {
const groupedFailures: { [k: string]: RuleFailure[] } = {};
for (const failure of failures) {
const fileName = failure.getFileName();
if (groupedFailures[fileName] !== undefined) {
groupedFailures[fileName].push(failure);
} else {
groupedFailures[fileName] = [failure];
}
}

output += "</checkstyle>";
return output;
const formattedFiles = fileNames.map(fileName => {
const formattedFailures =
groupedFailures[fileName] !== undefined
? groupedFailures[fileName].map(f => this.formatFailure(f))
: [];
const joinedFailures = formattedFailures.join(""); // may be empty
return `<file name="${this.escapeXml(fileName)}">${joinedFailures}</file>`;
});
const joinedFiles = formattedFiles.join("");
return `<?xml version="1.0" encoding="utf-8"?><checkstyle version="4.3">${joinedFiles}</checkstyle>`;
}

private formatFailure(failure: RuleFailure): string {
const line = failure.getStartPosition().getLineAndCharacter().line + 1;
const column = failure.getStartPosition().getLineAndCharacter().character + 1;
const severity = failure.getRuleSeverity();
const message = this.escapeXml(failure.getFailure());
// checkstyle parser wants "source" to have structure like <anything>dot<category>dot<type>
const source = `failure.tslint.${this.escapeXml(failure.getRuleName())}`;

return `<error line="${line}" column="${column}" severity="${severity}" message="${message}" source="${source}" />`;
}

private escapeXml(str: string): string {
Expand Down
6 changes: 5 additions & 1 deletion src/language/formatter/abstractFormatter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@ 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;

protected sortFailures(failures: RuleFailure[]): RuleFailure[] {
return failures.slice().sort(RuleFailure.compare);
Expand Down
3 changes: 2 additions & 1 deletion src/language/formatter/formatter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,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 the file paths that were linted
*/
format(failures: RuleFailure[], fixes?: RuleFailure[]): string;
format(failures: RuleFailure[], fixes?: RuleFailure[], fileNames?: string[]): string;

Choose a reason for hiding this comment

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

again, pretty sure the params should not be marked optional

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
4 changes: 3 additions & 1 deletion src/linter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ export class Linter {

private failures: RuleFailure[] = [];
private fixes: RuleFailure[] = [];
private readonly fileNames: string[] = [];

constructor(private readonly options: ILinterOptions, private program?: ts.Program) {
if (typeof options !== "object") {
Expand All @@ -139,6 +140,7 @@ export class Linter {
if (isFileExcluded(fileName, configuration)) {
return;
}
this.fileNames.push(fileName);
const sourceFile = this.getSourceFile(fileName, source);
const isJs = /\.jsx?$/i.test(fileName);
const enabledRules = this.getEnabledRules(configuration, isJs);
Expand Down Expand Up @@ -191,7 +193,7 @@ export class Linter {
}
const formatter = new Formatter();

const output = formatter.format(failures, this.fixes);
const output = formatter.format(failures, this.fixes, this.fileNames);

const errorCount = errors.length;
return {
Expand Down
11 changes: 9 additions & 2 deletions test/formatters/checkstyleFormatterTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { createFailure } from "./utils";
describe("Checkstyle Formatter", () => {
const TEST_FILE_1 = "formatters/jsonFormatter.test.ts"; // reuse existing sample file
const TEST_FILE_2 = "formatters/pmdFormatter.test.ts"; // reuse existing sample file
const TEST_FILE_3 = "formatters/proseFormatter.test.ts"; // reuse existing sample file
let sourceFile1: ts.SourceFile;
let sourceFile2: ts.SourceFile;
let formatter: IFormatter;
Expand Down Expand Up @@ -79,6 +80,7 @@ describe("Checkstyle Formatter", () => {
undefined,
"warning",
),
// no failures for file 3
];
// tslint:disable max-line-length
const expectedResult = `<?xml version="1.0" encoding="utf-8"?>
Expand All @@ -93,13 +95,18 @@ describe("Checkstyle Formatter", () => {
<error line="1" column="3" severity="warning" message="&amp;&lt;&gt;&#39;&quot; should be escaped" source="failure.tslint.escape" />
<error line="6" column="3" severity="warning" message="last failure" source="failure.tslint.last-name" />
</file>
<file name="${TEST_FILE_3}">
</file>
</checkstyle>`.replace(/>\s+/g, ">"); // Remove whitespace between tags

assert.equal(formatter.format(failures), expectedResult);
assert.equal(
formatter.format(failures, [], [TEST_FILE_1, TEST_FILE_2, TEST_FILE_3]),
expectedResult,
);
});

it("handles no failures", () => {
const result = formatter.format([]);
const result = formatter.format([], [], []);
assert.deepEqual(
result,
'<?xml version="1.0" encoding="utf-8"?><checkstyle version="4.3"></checkstyle>',
Expand Down