Added possibility to enforce different file name casings #4206

cheeZery · 2018-10-07T16:56:47Z

PR checklist

Addresses an existing issue: [Feature Request] Support pattern matching for rule "file-name-casing" #4071
Enhancement
- Includes tests
Documentation update

Overview of change:

Enhanced file-name-casing rule by adding possibility to enforce casing per file name match (case-insentive RegExp).

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

Since this is my first contribution to this repo, I'd like you to be very thoroughly in your review :-) I also wasn't familiar with the syntax for the options object, so that might be improvable.
And I only now see, that there's another PR (#4204, @rwaskiewicz) which also enhances this very rule by adding an exclusion option. Maybe that feature and this one can be combined by adding an additional "ignore" option to the file matcher argument. For example:

"file-name-casing": [true, [
    "index\.ts": "ignore",
    "\.ts$": "camel-case",
]];

By the way, I simply defined the RegExp as being case-insensitive but this can of course easily be changed. The check will currently also only consider the file name and its extension. I thought about taking the whole path in consideration but I guess that would have been more complicated due to two reasons: the different file separators (/ vs. \), and that we probably should use the relative path from the project base dir or the tslint.json file

In addition to that please consider the fact, that there are now some cases were the rule could produce false-positives for invalid rule arguments.
For example when the rule's arguments are [true, "camelCase"] (instead of "camel-case") the linter will now ignore this option. Prior to my change this was somewhat of a false-negative, because TSLint printed the error "File name must be undefined" (for every file).
And when the user now enters an invalid RegExp as a file match, that entry will be ignored.

Can and should the user be informed about those things with some kind of warning?

(Last but not least I stumbled upon a type in the launch.json and fixed it, hope that was fine)

CHANGELOG.md entry:

[new-rule-option] File name whitelist for file-name-casing

palantirtech · 2018-10-07T16:56:48Z

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

JoshuaKGoldberg · 2018-11-07T06:01:40Z

src/rules/fileNameCasingRule.ts

+    private static findApplicableCasing(
+        fileBaseName: string,
+        fileNameCasings: FileNameCasings
+    ): Casing | null {


| 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.

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!

JoshuaKGoldberg · 2018-11-07T06:03:39Z

src/rules/fileNameCasingRule.ts

+                [
+                    [".style.ts$", Casing.KebabCase],
+                    [".tsx$", Casing.PascalCase],
+                    [".*", Casing.CamelCase]


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.

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 😄

JoshuaKGoldberg · 2018-11-07T06:04:27Z

src/rules/fileNameCasingRule.ts

    };
    /* 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) {


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.

JoshuaKGoldberg · 2018-11-07T06:07:33Z

src/rules/fileNameCasingRule.ts

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


...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.

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 🌝

@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?

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.

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

JoshuaKGoldberg · 2018-11-07T06:09:18Z

Can and should the user be informed about those things with some kind of warning?

A console.warn seems like a good step in this direction, though agreed that there's not much precedent here.

Great first contribution, by the way! 😊

Use key-value pair instead of tuple; several performance improvements; fix options JSON schema; show warnings in case of unexpected options

JoshuaKGoldberg · 2018-11-12T21:59:04Z

.vscode/launch.json

@@ -5,15 +5,13 @@
            "name": "Debug CLI",
            "type": "node",
            "request": "launch",
-            "program": "${workspaceRoot}/build/src/tslint-cli.js",
+            "program": "${workspaceRoot}/build/src/tslintCli.js",


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

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

JoshuaKGoldberg · 2018-11-12T22:16:56Z

src/rules/fileNameCasingRule.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


The array again

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

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!

Will be done in separate PR

cheeZery · 2018-11-13T07:05:30Z

@JoshuaKGoldberg, I also now saw the duplicate PR #4280. Both PR pretty much do the same, in #4280 there are a more functions "outsourced" and a bit more types, here you have some additional checks and console warnings. I'm not sure which one should be merged 🤷‍♂️
Maybe we can get the best of both by working together in on one PR :)

yordis · 2018-11-16T06:31:59Z

here you have some additional checks and console warnings.

I love this, and I would propose to merge that part of the PR into my version (with your commits sign off @cheeZery I wouldn't like it otherwise to be honest) also the extra testings.

Also the other PR do not have extra changes unrelated to the issue so should be simple to review.

The functions could be move into the class definition if you want, but I would say I prefer to have the concept of the different validators as I did. Specially that reading each body of the functions you could map your head around it easier.

validate exists because it is a factory function basically responsable for deciding what validatio you should use.

Technically we are doing also the same but I decided to move different sections into functions separating the responsabilities.

yordis · 2018-11-21T18:28:33Z

@JoshuaKGoldberg sorry I bother you with this but should we do about this?

Whichever is the case I would love to have this config published so I can start using it on my packages but we need your direction and support on this.

cheeZery · 2018-11-22T17:16:17Z

@yordis I too like the idea to get the best of both PRs. I guess I even like your approach, to define the functions outside of the class scope better!
I wouldn't mind if your PR would be merged first and after that we take a look and figure out what of my PR is still needed and merge that afterwards :-)

JoshuaKGoldberg · 2018-11-22T19:19:03Z

Great, let's do that! Thanks you two 😍

In summary, I'll:

Merge in Add support for complex matching to file-name-casing-rule #4284
Resolve merge conflicts on this PR
Re-review this one, which should just be the added rule option validation

cheeZery · 2018-11-22T19:46:32Z

Perfect!

JoshuaKGoldberg · 2018-11-22T20:56:44Z

@cheeZery does this merge look good to you?

yordis · 2018-11-22T22:24:22Z

LOVE IT ❤️ 💟 Best of both PRs 💟 ❤️ 👏

cheeZery · 2018-11-23T06:36:04Z

@JoshuaKGoldberg it does indeed look good to me 😊 I agree with @yordis 👬 Thanks to the both of you!

JoshuaKGoldberg · 2018-11-23T06:59:18Z

Awesome, thanks everyone!

cheeZery added 2 commits October 7, 2018 17:57

Enhance file name casing rule and improved its tests

4f57de1

Added option description and examples

cf74d36

JoshuaKGoldberg suggested changes Nov 7, 2018

View reviewed changes

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

Utilize undefined instead of explicitly using null

d40b082

JoshuaKGoldberg mentioned this pull request Nov 8, 2018

Better handling of invalid rule configuration options #4280

Closed

JoshuaKGoldberg added PR: waiting for author and removed PR: waiting for reviewer labels Nov 8, 2018

cheeZery added 2 commits November 12, 2018 22:36

Change file-name-casing option to key-value pair

96d92db

Use key-value pair instead of tuple; several performance improvements; fix options JSON schema; show warnings in case of unexpected options

Merge branch 'master' into feature/file-name-casing-enhancement

de96576

JoshuaKGoldberg reviewed Nov 12, 2018

View reviewed changes

cheeZery added 2 commits November 13, 2018 07:21

Adjust description and example for object option

59f16da

Undo fix in launch.json

da5755e

Will be done in separate PR

Merge branch 'master' into feature/file-name-casing-enhancement

feb268d

JoshuaKGoldberg added PR: waiting for reviewer and removed PR: waiting for author labels Nov 22, 2018

JoshuaKGoldberg approved these changes Nov 22, 2018

View reviewed changes

JoshuaKGoldberg merged commit a43cfdc into palantir:master Nov 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added possibility to enforce different file name casings #4206

Added possibility to enforce different file name casings #4206

cheeZery commented Oct 7, 2018 •

edited

palantirtech commented Oct 7, 2018

JoshuaKGoldberg Nov 7, 2018

cheeZery Nov 7, 2018

JoshuaKGoldberg Nov 7, 2018

cheeZery Nov 7, 2018

JoshuaKGoldberg Nov 7, 2018

JoshuaKGoldberg Nov 7, 2018

cheeZery Nov 7, 2018

cheeZery Nov 8, 2018

JoshuaKGoldberg Nov 8, 2018

cheeZery Nov 12, 2018 •

edited

JoshuaKGoldberg commented Nov 7, 2018 •

edited

JoshuaKGoldberg Nov 12, 2018

cheeZery Nov 13, 2018

JoshuaKGoldberg Nov 12, 2018

cheeZery Nov 13, 2018

cheeZery commented Nov 13, 2018 •

edited

yordis commented Nov 16, 2018

yordis commented Nov 21, 2018

cheeZery commented Nov 22, 2018

JoshuaKGoldberg commented Nov 22, 2018

cheeZery commented Nov 22, 2018

JoshuaKGoldberg commented Nov 22, 2018

yordis commented Nov 22, 2018 •

edited

cheeZery commented Nov 23, 2018 •

edited

JoshuaKGoldberg commented Nov 23, 2018

Added possibility to enforce different file name casings #4206

Added possibility to enforce different file name casings #4206

Conversation

cheeZery commented Oct 7, 2018 • edited

PR checklist

Overview of change:

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

CHANGELOG.md entry:

palantirtech commented Oct 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheeZery Nov 12, 2018 • edited

Choose a reason for hiding this comment

JoshuaKGoldberg commented Nov 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheeZery commented Nov 13, 2018 • edited

yordis commented Nov 16, 2018

yordis commented Nov 21, 2018

cheeZery commented Nov 22, 2018

JoshuaKGoldberg commented Nov 22, 2018

cheeZery commented Nov 22, 2018

JoshuaKGoldberg commented Nov 22, 2018

yordis commented Nov 22, 2018 • edited

cheeZery commented Nov 23, 2018 • edited

JoshuaKGoldberg commented Nov 23, 2018

cheeZery commented Oct 7, 2018 •

edited

cheeZery Nov 12, 2018 •

edited

JoshuaKGoldberg commented Nov 7, 2018 •

edited

cheeZery commented Nov 13, 2018 •

edited

yordis commented Nov 22, 2018 •

edited

cheeZery commented Nov 23, 2018 •

edited