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

Add new rules no-missing-message-ids and no-unused-message-ids #254

Merged
merged 2 commits into from
Jul 1, 2022

Conversation

bmish
Copy link
Member

@bmish bmish commented Jun 21, 2022

Fixes #57.

Detects:

  • Using a messageId that does not exist
  • Leaving a message in meta.messages that is unused

These were implemented as separate rules because:

  • Each is quite complicated on its own and it helps to be able to test them in isolation
  • In the event of false positives, the user can just disable one of the rules but not the other

Note that I eliminated many possible false positive scenarios by testing these rules on the ESLint codebase.

TODO

  • Fix failing test case
  • Write unit tests for util functions
  • Add JSdoc for util functions
  • Rename to require-valid-message-ids
  • Split into separate rules no-missing-message-ids and no-unused-message-ids since the unused message ID check can have false positives when variables or helper functions are used
  • Test on ESLint repository to detect edge cases
  • Ignore usage check when can't statically determine messageIds used like below examples:
function reportEnd(node, messageId) {
    context.report({
        node,
        messageId
    });
}
  • Consider variable messageId:
let messageId;
if (foo) {
  messageId = 'abc';
} else {
  messageId = 'def';
}

context.report({
  node,
  messageId
});
  • Handle conditional in suggestions
context.report({
    node,
    messageId: "unexpectedRegExp",
    suggest: noFix ? [] : [{
        messageId: "replaceWithLiteral",
    }]
});
  • Try to handle some false positives in eslint-plugin-unicorn with dynamic object keys:
const MESSAGE_ID = 'no-this-assignment';
const messages = {
	[MESSAGE_ID]: 'Do not assign `this` to `{{name}}`.',
};

After this is merged:

@bmish bmish added the feature label Jun 21, 2022
@bmish bmish force-pushed the no-invalid-message-ids branch 2 times, most recently from 43a3550 to d91d1a0 Compare June 29, 2022 23:55
@bmish bmish changed the title Add new rule no-invalid-message-ids Add new rules no-missing-message-ids and no-unused-message-ids Jun 29, 2022
@bmish bmish force-pushed the no-invalid-message-ids branch 4 times, most recently from 806decb to c1e6abf Compare June 30, 2022 17:22
@bmish bmish force-pushed the no-invalid-message-ids branch from c1e6abf to da64dc0 Compare June 30, 2022 17:31
@bmish bmish marked this pull request as ready for review June 30, 2022 17:32
@bmish
Copy link
Member Author

bmish commented Jun 30, 2022

This is ready for review. These rules were fairly-challenging to implement given the number of false positives originally when I ran them on the ESLint codebase but I've solved all the issues.

@aladdin-add aladdin-add self-requested a review July 1, 2022 05:20
Copy link
Contributor

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

Looks so great! 💯

I've tested it in a few repos:

  • eslint
  • eslint-plugin-node
  • eslint-plugin-react
  • acorn (to find unexpected crashes on no-eslint-rule js)

It's working very well, thanks! 👍

fixable: null,
schema: [],
messages: {
missingMessage: '`meta.messages` is missing this `messageId`.',
Copy link
Contributor

Choose a reason for hiding this comment

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

could the message be clearer, e.g. messageId "foo"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes updated.

lib/rules/no-unused-message-ids.js Outdated Show resolved Hide resolved
@bmish
Copy link
Member Author

bmish commented Jul 1, 2022

@aladdin-add updated the messages and I also fixed some false positives affecting eslint-plugin-unicorn due to variable message object keys.

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.

Rule: valid usage of messageId
2 participants