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

Added possibility to enforce different file name casings #4206

Merged
merged 8 commits into from Nov 23, 2018
Merged

Added possibility to enforce different file name casings #4206

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4f57de1
Enhance file name casing rule and improved its tests
cheeZery Oct 7, 2018
cf74d36
Added option description and examples
cheeZery Oct 7, 2018
d40b082
Utilize undefined instead of explicitly using null
cheeZery Nov 7, 2018
96d92db
Change file-name-casing option to key-value pair
cheeZery Nov 12, 2018
de96576
Merge branch 'master' into feature/file-name-casing-enhancement
cheeZery Nov 12, 2018
59f16da
Adjust description and example for object option
cheeZery Nov 13, 2018
da5755e
Undo fix in launch.json
cheeZery Nov 13, 2018
feb268d
Merge branch 'master' into feature/file-name-casing-enhancement
Nov 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 5 additions & 13 deletions .vscode/launch.json
View file
Open in desktop
Expand Up @@ -5,15 +5,13 @@
"name": "Debug CLI",
"type": "node",
"request": "launch",
"program": "${workspaceRoot}/build/src/tslint-cli.js",
"program": "${workspaceRoot}/build/src/tslintCli.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh, mind taking these changes out into a separate PR? Definitely a bug in launch.json that the file hasn't been renamed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't mind at all, it's better to separate those things. Reverted my changes and will open a new PR for that fix.

"stopOnEntry": false,
"args": [],
"cwd": "${workspaceRoot}",
"preLaunchTask": "tsc",
"runtimeExecutable": null,
"runtimeArgs": [
"--nolazy"
],
"runtimeArgs": ["--nolazy"],
"env": {
"NODE_ENV": "development"
},
Expand All @@ -32,9 +30,7 @@
"cwd": "${workspaceRoot}",
"preLaunchTask": "tsc",
"runtimeExecutable": null,
"runtimeArgs": [
"--nolazy"
],
"runtimeArgs": ["--nolazy"],
"env": {
"NODE_ENV": "development"
},
Expand All @@ -53,9 +49,7 @@
"cwd": "${workspaceRoot}",
"preLaunchTask": "tsc",
"runtimeExecutable": null,
"runtimeArgs": [
"--nolazy"
],
"runtimeArgs": ["--nolazy"],
"env": {
"NODE_ENV": "development"
},
Expand All @@ -74,9 +68,7 @@
"cwd": "${workspaceRoot}",
"preLaunchTask": "tsc",
"runtimeExecutable": null,
"runtimeArgs": [
"--nolazy"
],
"runtimeArgs": ["--nolazy"],
"env": {
"NODE_ENV": "development"
},
Expand Down
124 changes: 105 additions & 19 deletions src/rules/fileNameCasingRule.ts
View file
Open in desktop
Expand Up @@ -25,47 +25,95 @@ enum Casing {
CamelCase = "camel-case",
PascalCase = "pascal-case",
KebabCase = "kebab-case",
SnakeCase = "snake-case",
SnakeCase = "snake-case"
}

type FileNameRegExpWithCasing = [string, Casing];
type FileNameCasings = FileNameRegExpWithCasing[];

export class Rule extends Lint.Rules.AbstractRule {
/* tslint:disable:object-literal-sort-keys */
public static metadata: Lint.IRuleMetadata = {
ruleName: "file-name-casing",
description: "Enforces a consistent file naming convention",
rationale: "Helps maintain a consistent style across a file hierarchy",
optionsDescription: Lint.Utils.dedent`
One of the following arguments must be provided:
One argument which is either a string defining the file casing or an array consisting of a file name
matches and corresponding casing strings.

* \`${Casing.CamelCase}\`: File names must be camel-cased: \`fileName.ts\`.
* \`${Casing.PascalCase}\`: File names must be Pascal-cased: \`FileName.ts\`.
* \`${Casing.KebabCase}\`: File names must be kebab-cased: \`file-name.ts\`.
* \`${Casing.SnakeCase}\`: File names must be snake-cased: \`file_name.ts\`.`,
* In both cases the casing string must be one of the options:
** \`${Casing.CamelCase}\`: File names must be camel-cased: \`fileName.ts\`.
** \`${Casing.PascalCase}\`: File names must be pascal-cased: \`FileName.ts\`.
** \`${Casing.KebabCase}\`: File names must be kebab-cased: \`file-name.ts\`.
** \`${Casing.SnakeCase}\`: File names must be snake-cased: \`file_name.ts\`.

* The array again consists of array with two items. The first item must be a case-insenstive
Copy link
Contributor

Choose a reason for hiding this comment

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

The array again

Should this be updated to mention that it takes in an object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, forgot that, thanks. I changed it now but since English isn't my first language feel free to point out anything that could be phrased better!

regular expression to match files, the second item must be a valid casing option (see above)`,
options: {
type: "array",
items: [
{
type: "string",
enum: [Casing.CamelCase, Casing.PascalCase, Casing.KebabCase, Casing.SnakeCase],
},
],
type: "list",
listType: {
anyOf: [
{
type: "string",
enum: [
Casing.CamelCase,
Casing.PascalCase,
Casing.KebabCase,
Casing.SnakeCase
]
},
{
type: "array",
items: [
{
type: "string"
},
{
type: "string",
enum: [
Casing.CamelCase,
Casing.PascalCase,
Casing.KebabCase,
Casing.SnakeCase
]
}
]
}
]
}
},
optionExamples: [
[true, Casing.CamelCase],
[true, Casing.PascalCase],
[true, Casing.KebabCase],
[true, Casing.SnakeCase],
[
true,
[
[".style.ts$", Casing.KebabCase],
[".tsx$", Casing.PascalCase],
[".*", Casing.CamelCase]
Copy link
Contributor

Choose a reason for hiding this comment

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

It's generally convention to use objects for key/value pairs. Unless you have a specific need, please switch to something like:

{
    ".style.ts$": Casing.KebabCase,
    ".tsx$": Casing.PascalCase,
    ".*": Casing.CamelCase,
}

One benefit, in addition to being semantically more clear, is that you get rid of duplicate keys easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will switch to object, thanks!
I'm embarassed to admit how long it took me to realize that the syntax in options is actually JSON schema (since I never really worked with that). After that I optimized the declaration a bit, looking forward to your opinion after I pushed my changes.
Maybe the comment for options in IRuleMetadata should begin with "JSON schema ..." for people like me 😄

]
]
],
hasFix: false,
type: "style",
typescriptOnly: false,
typescriptOnly: false
};
/* tslint:enable:object-literal-sort-keys */

private static FAILURE_STRING(expectedCasing: Casing): string {
return `File name must be ${Rule.stylizedNameForCasing(expectedCasing)}`;
}

private static isValidCasingOption(casing: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: instead of this dedicated function, you can keep a const validCasingOptions = new Set([Casing.CamelCase, ...]); outside of the class and use its .has. A little less code.

return (
[Casing.CamelCase, Casing.KebabCase, Casing.PascalCase, Casing.SnakeCase].indexOf(
casing as Casing
) !== -1
);
}

private static stylizedNameForCasing(casing: Casing): string {
switch (casing) {
case Casing.CamelCase:
Expand All @@ -79,7 +127,7 @@ export class Rule extends Lint.Rules.AbstractRule {
}
}

private static isCorrectCasing(fileName: string, casing: Casing): boolean {
private static hasFileNameCorrectCasing(fileName: string, casing: Casing): boolean {
switch (casing) {
case Casing.CamelCase:
return isCamelCased(fileName);
Expand All @@ -92,15 +140,53 @@ export class Rule extends Lint.Rules.AbstractRule {
}
}

private static isValidRegExp(regExpString: string) {
try {
RegExp(regExpString);
return true;
} catch (e) {
return false;
}
}

private static findApplicableCasing(
fileBaseName: string,
fileNameCasings: FileNameCasings
): Casing | null {
Copy link
Contributor

Choose a reason for hiding this comment

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

| null

Why so many nulls instead of undefined? It's generally preferable in JavaScript/TypeScript to prefer using undefined, as it's the default value. TSLint typically goes by that preference.

Unless you have a real need for using null, please switch to undefined as defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, in my team I'm still fighting for strictNullChecks, so as of now we don't really explicitly work with either undefined or null. But we're getting there 😊 While changing the code to use undefined I realized that's way more intuitive, thanks for the suggestion!

const applicableCasing = fileNameCasings.find(fileNameCasing => {
const fileNameMatch = fileNameCasing[0];
return (
Rule.isValidRegExp(fileNameMatch) && RegExp(fileNameMatch, "i").test(fileBaseName)
Copy link
Contributor

Choose a reason for hiding this comment

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

...so if an invalid regular expression is provided, the rule will fail silently?

We should prefer to fail verbosely for invalid configurations. Either remove the isValidRegExp check (+1) or console.warn if it fails.

There are a lot of ways to provide invalid configurations, and it's near-impossible to guard against all of them. TSLint is moving towards standardized config options but it'll be a while before they're ready.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, there already were cases like this in the current implementation, see here for example: if the mandatory casing wasn't provided simply return an empty array 🤷‍♂️
I would like to fail more verbosely as you suggested, hence my question that you already answered below.
Currently there don't seem to be any console.warn's in the rules, so maybe there's a better way? In objectLiteralSortKeys for example an actual Error is thrown.
But I will go with what you guys suggest 🌝

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JoshuaKGoldberg I just realized that it might be not a good idea to either throw an error or use console.warn, since this would happen for every linted file in the project. Using showWarningOnce on the other should work, some rules even use this already. Any other suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Filed #4280 to tackle this more broadly.

In the meantime, feel free to either use the showWarningOnce approach or not validate. IMO it shouldn't be each rule's responsibility to validate options ideally, since they already specify their format in their metadata... but since there isn't yet a method for that, it seems up to you.

Copy link
Contributor Author

@cheeZery cheeZery Nov 12, 2018 •
edited

Choose a reason for hiding this comment

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

Thanks for the issue! (There's a small typo in my mention 😌) I now added some uses of showWarningOnce

);
});

return applicableCasing !== undefined ? applicableCasing[1] : null;
}

public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
if (this.ruleArguments.length !== 1) {
return [];
}

const casing = this.ruleArguments[0] as Casing;
const fileName = path.parse(sourceFile.fileName).name;
if (!Rule.isCorrectCasing(fileName, casing)) {
return [new Lint.RuleFailure(sourceFile, 0, 0, Rule.FAILURE_STRING(casing), this.ruleName)];
let casing: Casing | null = null;

const parsedPath = path.parse(sourceFile.fileName);

if (typeof this.ruleArguments[0] === "object") {
casing = Rule.findApplicableCasing(parsedPath.base, this
.ruleArguments[0] as FileNameCasings);
} else if (typeof this.ruleArguments[0] === "string") {
casing = this.ruleArguments[0] as Casing;
}

if (casing === null || !Rule.isValidCasingOption(casing)) {
return [];
}

if (!Rule.hasFileNameCorrectCasing(parsedPath.name, casing)) {
return [
new Lint.RuleFailure(sourceFile, 0, 0, Rule.FAILURE_STRING(casing), this.ruleName)
];
}

return [];
Expand Down
0 ...ile-name-casing/default/camelCase.ts.lint → ...-name-casing/camel-case/camelCase.ts.lint
View file
Open in desktop
File renamed without changes.
0 ...name-casing/default/no-camel-case.ts.lint → ...e-casing/camel-case/no-camel-case.ts.lint
View file
Open in desktop
File renamed without changes.
5 changes: 5 additions & 0 deletions test/rules/file-name-casing/camel-case/tslint.json
View file
Open in desktop
@@ -0,0 +1,5 @@
{
"rules": {
"file-name-casing": [true, "camel-case"]
}
}
5 changes: 0 additions & 5 deletions test/rules/file-name-casing/default/tslint.json
View file
Open in desktop

This file was deleted.

Empty file added test/rules/file-name-casing/file-matcher/PascalCase.ts.lint
View file
Open in desktop
Empty file.
Empty file added test/rules/file-name-casing/file-matcher/camelCase.ts.lint
View file
Open in desktop
Empty file.
2 changes: 2 additions & 0 deletions test/rules/file-name-casing/file-matcher/notPascalCase.ts.lint
View file
Open in desktop
@@ -0,0 +1,2 @@

~nil [File name must be PascalCase]
2 changes: 2 additions & 0 deletions test/rules/file-name-casing/file-matcher/not_camel_case.ts.lint
View file
Open in desktop
@@ -0,0 +1,2 @@

~nil [File name must be camelCase]
2 changes: 2 additions & 0 deletions test/rules/file-name-casing/file-matcher/not_kebab_case.tsx.lint
View file
Open in desktop
@@ -0,0 +1,2 @@

~nil [File name must be kebab-case]
13 changes: 13 additions & 0 deletions test/rules/file-name-casing/file-matcher/tslint.json
View file
Open in desktop
@@ -0,0 +1,13 @@
{
"rules": {
"file-name-casing": [
true,
[
[".*)", "snake-case"], // making sure invalid RegExp will be ignored
["pascal.?case", "pascal-case"],
["tsx$", "kebab-case"],
[".*", "camel-case"]
]
]
}
}
Empty file added test/rules/file-name-casing/invalid-option/snake_case.ts.lint
View file
Open in desktop
Empty file.
5 changes: 5 additions & 0 deletions test/rules/file-name-casing/invalid-option/tslint.json
View file
Open in desktop
@@ -0,0 +1,5 @@
{
"rules": {
"file-name-casing": [true, "invalid-option"]
}
}
Empty file added test/rules/file-name-casing/kebab-case/kebab-case.ts.lint
View file
Open in desktop
Empty file.
2 changes: 2 additions & 0 deletions test/rules/file-name-casing/kebab-case/noKebabCase.ts.lint
View file
Open in desktop
@@ -0,0 +1,2 @@

~nil [File name must be kebab-case]
5 changes: 5 additions & 0 deletions test/rules/file-name-casing/kebab-case/tslint.json
View file
Open in desktop
@@ -0,0 +1,5 @@
{
"rules": {
"file-name-casing": [true, "kebab-case"]
}
}
Empty file added test/rules/file-name-casing/pascal-case/PascalCase.ts.lint
View file
Open in desktop
Empty file.
2 changes: 2 additions & 0 deletions test/rules/file-name-casing/pascal-case/no-pascal-case.ts.lint
View file
Open in desktop
@@ -0,0 +1,2 @@

~nil [File name must be PascalCase]
5 changes: 5 additions & 0 deletions test/rules/file-name-casing/pascal-case/tslint.json
View file
Open in desktop
@@ -0,0 +1,5 @@
{
"rules": {
"file-name-casing": [true, "pascal-case"]
}
}
2 changes: 0 additions & 2 deletions test/rules/file-name-casing/snake-case/no-camel-case.ts.lint
View file
Open in desktop

This file was deleted.

0 ...-name-casing/snake-case/NoPascalCase.lint → ...e-casing/snake-case/no-snake-case.ts.lint
View file
Open in desktop
File renamed without changes.