Skip to content

Commit

Permalink
Pass all listed file names to formatter.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mastermatt committed Oct 26, 2018
1 parent 237fd90 commit ad2e789
Show file tree
Hide file tree
Showing 5 changed files with 39 additions and 33 deletions.
55 changes: 27 additions & 28 deletions src/formatters/checkstyleFormatter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export class Formatter extends AbstractFormatter {
formatterName: "checkstyle",
description: "Formats errors as through 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 @@ -39,36 +39,35 @@ 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 (fileName in groupedFailures) {
groupedFailures[fileName].push(failure);
} else {
groupedFailures[fileName] = [failure];
}
}

output += "</checkstyle>";
return output;
const formattedFiles = fileNames.map((fileName) => {
const formattedFailures = fileName in groupedFailures ? 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
2 changes: 1 addition & 1 deletion src/language/formatter/abstractFormatter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

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 @@ -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
*/
format(failures: RuleFailure[], fixes?: RuleFailure[]): string;
format(failures: RuleFailure[], fixes?: RuleFailure[], fileNames?: string[]): string;
}
4 changes: 3 additions & 1 deletion src/linter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ export class Linter {

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

/**
* Creates a TypeScript program object from a tsconfig.json file path and optional project directory.
Expand Down Expand Up @@ -116,6 +117,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 @@ -156,7 +158,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
8 changes: 6 additions & 2 deletions test/formatters/checkstyleFormatterTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,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 All @@ -46,6 +47,7 @@ describe("Checkstyle Formatter", () => {
createFailure(sourceFile2, 0, 1, "first failure", "first-name", undefined, "error"),
createFailure(sourceFile2, 2, 3, "&<>'\" should be escaped", "escape", undefined, "warning"),
createFailure(sourceFile2, maxPosition2 - 1, maxPosition2, "last failure", "last-name", undefined, "warning"),
// no failures for file 3
];
// tslint:disable max-line-length
const expectedResult =
Expand All @@ -61,13 +63,15 @@ 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>');
});
});

0 comments on commit ad2e789

Please sign in to comment.