Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Rule: no-invalid-names #298

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

New Rule: no-invalid-names #298

wants to merge 2 commits into from

Conversation

platinumazure
Copy link
Owner

@platinumazure platinumazure commented Jan 3, 2023

I did not implement a pattern option-- figure that could be a future enhancement if there is demand.

Open questions:

  • Do we want to add linting for any newlines (CR, LF, CRLF) in the test/module names? Seems like that could be a good idea.

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.

@coveralls
Copy link

coveralls commented Jan 3, 2023

Coverage Status

coverage: 100.0%. remained the same
when pulling 3cd6210 on no-invalid-names
into 6251689 on main.

@bmish
Copy link
Collaborator

bmish commented Jan 3, 2023

You need to add "closes #203" to the PR description to correctly link the issue to the PR.

Comment on lines +28 to +37
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."
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

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 */
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, agreed.

}]
},
{
code: `${callee}(foo || "name", function () {});`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

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 () {});`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

foo + bar can be a legitimate way to concatenate strings so I believe it should be a valid test case.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I suppose. I'll allow it for plus only

`${callee}("simple valid name");`,
`${callee}("simple valid name", function () {});`,

// Cannot check variables
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 => [
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could have a ## Related section that mentions:

@bmish bmish changed the title New: no-invalid-names rule (closes #203) New Rule: no-invalid-names Jun 27, 2023
This was referenced Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Rule: no-invalid-names
3 participants