New Rule: no-invalid-names
#298
Conversation
|
You need to add "closes #203" to the PR description to correctly link the issue to the PR. |
| moduleNameEmpty: "Module name is empty.", | ||
| moduleNameInvalidType: "Module name \"{{ name }}\" is invalid type: {{ type }}.", | ||
| moduleNameMissing: "Module name is missing.", | ||
| moduleNameOuterQUnitDelimiters: "Module name \"{{ name }}\" has leading and/or trailing QUnit delimiter: (> or :).", | ||
| moduleNameOuterSpaces: "Module name has leading and/or trailing spaces.", | ||
| testNameEmpty: "Test name is empty.", | ||
| testNameInvalidType: "Test name \"{{ name }}\" is invalid type: {{ type }}.", | ||
| testNameMissing: "Test name is missing.", | ||
| testNameOuterQUnitDelimiters: "Test name \"{{ name }}\" has leading and/or trailing QUnit delimiter (> or :).", | ||
| testNameOuterSpaces: "Test name has leading and/or trailing spaces." |
There was a problem hiding this comment.
Personally, I think it would be better to pass objectType as a parameter (just like name and type) instead of duplicating these messages for module and test. That means fewer messageIds/duplication to manage, and also makes the messageIds more statically-analyzable for rules like eslint-plugin/no-unused-message-ids, which can't work when template strings like ${objectType}NameInvalidType are present.
There was a problem hiding this comment.
Yeah, that makes sense. Originally I had it in my head that localization would be harder with unified messages but now I realize that's not the case.
|
|
||
| return { | ||
| "CallExpression": function (node) { | ||
| /* istanbul ignore else: Correctly does nothing */ |
There was a problem hiding this comment.
I think it's actually important that we have test cases for the else case to ensure this rule does not run on unrelated functions:
foo();
foo(function() {});
foo(' string with leading and trailing spaces ', function() {}); | }] | ||
| }, | ||
| { | ||
| code: `${callee}(foo || "name", function () {});`, |
There was a problem hiding this comment.
If somebody wants to decide on a string with foo || "name", that should be allowed. It's a good way to set a fallback. I believe this should be a valid test case.
There was a problem hiding this comment.
I suppose, as long as one of the arguments is a string. That means I'll need to build in some recursion or something... Thanks for the feedback
There was a problem hiding this comment.
I think we need to be careful to allow the user to dynamically generate the name anyway they would like. That's one thing I learned working on this plugin and on eslint-plugin-eslint-plugin in particular -- users can be very creative in how they generate their data, and unnecessarily restricting what types of statements they can use will end up causing headaches (and lots of bug reports) later.
There was a problem hiding this comment.
That's one approach, I suppose, but I'm also a fan of telling people to use disable statements when they want to work around a rule in a convoluted way.
| }] | ||
| }, | ||
| { | ||
| code: `${callee}(foo + bar, function () {});`, |
There was a problem hiding this comment.
foo + bar can be a legitimate way to concatenate strings so I believe it should be a valid test case.
There was a problem hiding this comment.
I suppose. I'll allow it for plus only
| `${callee}("simple valid name");`, | ||
| `${callee}("simple valid name", function () {});`, | ||
|
|
||
| // Cannot check variables |
There was a problem hiding this comment.
Function call e.g. getTitle() should be tested as valid too.
| const ruleTester = new RuleTester(); | ||
|
|
||
| ruleTester.run("no-invalid-names", rule, { | ||
| valid: [...TEST_FUNCTIONS, ...MODULE_FUNCTIONS].flatMap(callee => [ |
There was a problem hiding this comment.
Template string should be a valid test case.
| ## When Not to Use It | ||
|
|
||
| This rule is mostly stylistic, but can cause problems in the case of QUnit delimiters | ||
| at the start and end of test names. |
There was a problem hiding this comment.
We could have a ## Related section that mentions:
bmish
changed the title
no-invalid-names
I did not implement a pattern option-- figure that could be a future enhancement if there is demand.
Open questions:
Note to reviewers: I did not want to keep repeating test cases, so I did some fun flatMaps to generate different permutations. Let me know if their readability can be improved.
Fixes #203.