Add support for custom messages to restricted imports #9413

majapw · 2017-10-12T00:55:50Z

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What rule do you want to change?
no-restricted-imports and no-restricted-modules (am interested in feedback first before making the changes to no-restricted-modules as well)

Does this change cause the rule to produce more or fewer warnings?
Same amount of errors but adds more nuance to the error messaging

How will the change be implemented? (New option, new default behavior, etc.)?
Modification to allowed options. Anywhere you can pass in a path string, you can now also pass in an object with a name and message key where the name corresponds to a path and the message is the custom message you want appended to the default error message.

Please provide some example code that this change will affect:
All existing configurations will continue to work. However, the following will also be accepted:

"no-restricted-imports": ["error", [{
  "name": "import-foo",
  "message": "Please use import-bar instead."
}]]

or like this:

"no-restricted-imports": ["error", {
  "paths": [{
    "name": "import-foo",
    "message": "Please use import-bar instead."
  }]
}]

What does the rule currently do for this code?
errors on any restricted imports that are passed into the options.

What will the rule do after it's changed?
continue to error on any restricted imports that are passed into the options, but with more clarity on what the developer should do instead.

What changes did you make? (Give an overview)
Following the precedent set by no-restricted-globals, anywhere you can pass a path string, you can also pass an object with a "name" and "message" key. This custom message will be appended to any error message triggered by an import of the "name" value. Custom messages are not supported with patterns.

Is there anything you'd like reviewers to focus on?
See if this pattern (and naming scheme) is acceptable so as to do the same for no-restricted-modules

FYI @ljharb @yzimet

jsf-clabot · 2017-10-12T00:55:51Z

All committers have signed the CLA.

eslintbot · 2017-10-12T00:55:52Z

Thanks for the pull request, @majapw! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

eslintbot · 2017-10-12T00:58:58Z

LGTM

ljharb

This follows the precedent from no-restricted-properties, and is a great change!

not-an-aardvark · 2017-10-12T02:07:31Z

Thanks for the PR. Have you seen #8400? It looks like a similar proposal was accepted to add both paths and patterns options to no-restricted-imports, to allow for custom messages to be used.

I think it would be fine to just add the paths option in this PR, but I want to make sure this PR is compatible with the plan in #8400 so that we would still be able to add patterns later.

ljharb · 2017-10-12T02:12:26Z

The plan for "paths" is identical in this PR to #8400; I definitely think this could be merged right now, and in no way would it block #8400.

ilyavolodin

Could you please sign our CLA? Unfortunately, we can't merge anything without signed CLA.

not-an-aardvark

This looks good aside from a couple of minor edge-cases.

not-an-aardvark · 2017-10-12T02:20:12Z

lib/rules/no-restricted-imports.js

+        function reportPath(node) {
+            const importName = node.source.value.trim();
+            const customMessage = restrictedPathMessages[importName];
+            const message = customMessage


This will cause the default message to get used if an empty string gets used as the custom message, which I'm assuming is unintended. (It's unlikely that users will provide an empty string very often, but I think it would be better to handle this case consistently with the case where non-empty strings are provided.)

Could that be resolved by modifying the JSON schema to forbid an empty message?

(specifically, by adding minLength: 1)

That would also work. I don't have any strong preferences for how empty strings are handled; I just want to make sure that the behavior is consistent with the rest of the rule (or that if the behavior is inconsistent, it's a design choice rather than a bug).

not-an-aardvark · 2017-10-12T02:21:36Z

lib/rules/no-restricted-imports.js

+         * @private
+         */
+        function isRestrictedPath(name) {
+            return restrictedPathMessages.hasOwnProperty(name);


I think this will cause an error to get thrown if someone uses the string "hasOwnProperty" as a path. It would be better to use something like Object.prototype.hasOwnProperty.call(restrictedPathMessages, name).

eslintbot · 2017-10-12T17:34:27Z

LGTM

majapw · 2017-10-12T17:35:56Z

@not-an-aardvark I think I've addressed your concerns (hasOwnProperty on the restrictedPathMessages object and add a minLength to the schema for the custom message). I've also duplicated the effort for no-restricted-modules. Please take another look!

ljharb

(ﾉ◕ヮ◕)ﾉ*:・ﾟ✧

not-an-aardvark

Looks good aside from a typo in the documentation.

not-an-aardvark · 2017-10-12T18:33:32Z

docs/rules/no-restricted-modules.md

+You may also specify a custom message for any paths you want to restrict as follows:
+
+```json
+"no-restricted-imports": ["error", [{


I think these examples should say no-restricted-modules rather than no-restricted-imports.

Oh whoops! My copy-pasta bad

eslintbot · 2017-10-12T23:17:57Z

LGTM

majapw · 2017-10-12T23:18:11Z

@not-an-aardvark addressed!

not-an-aardvark

Looks good to me. Thanks for contributing!

platinumazure

Just requesting some minor changes around doc typos and comment inconsistencies, otherwise looks great! Thanks for contributing!

platinumazure · 2017-10-12T23:37:38Z

docs/rules/no-restricted-imports.md

+}]
+```
+
+The custom message will be appends to the default error message. Please note that you may not specify custom error messages for restricted patterns as a particular import may match more than one pattern.


I think this should say "The custom message will be appended to the default error message."

platinumazure · 2017-10-12T23:38:24Z

docs/rules/no-restricted-modules.md

+}]
+```
+
+The custom message will be appends to the default error message. Please note that you may not specify custom error messages for restricted patterns as a particular module may match more than one pattern.


I think this should say "The custom message will be appended to the default error message."

platinumazure · 2017-10-12T23:42:00Z

lib/rules/no-restricted-imports.js

+        }
+
+        /**
+         * Check if the given name is a restricted global name.


Could this comment be edited? (This seems to have been copied from no-restricted-globals)

platinumazure · 2017-10-12T23:42:38Z

lib/rules/no-restricted-modules.js

+        }
+
+        /**
+         * Check if the given name is a restricted global name.


Could this comment be edited? (This seems to have been copied from no-restricted-globals)

kaicataldo

Aside from the few suggestions by @platinumazure, this LGTM. Thanks for contributing!

kaicataldo · 2017-10-13T22:15:54Z

Let's make sure to update the corresponding issue after this lands so that it's clear that only custom messages for patterns are needed.

Following the precedent set by no-restricted-globals, anywhere you can pass a path string, you can also pass an object with a "name" and "message" key. This custom message will be appended to any error message triggered by an import of the "name" value. Custom messages are not supported with patterns.

eslintbot · 2017-10-13T23:41:11Z

LGTM

majapw · 2017-10-13T23:42:13Z

@platinumazure I believe I've addressed your comments as well.

platinumazure

LGTM, thanks for contributing!

kaicataldo · 2017-10-14T04:28:10Z

Thanks for contributing to ESLint!

majapw force-pushed the maja-add-custom-message-to-no-restricted-imports branch from 870c505 to d99da30 Compare October 12, 2017 00:58

ljharb approved these changes Oct 12, 2017

View reviewed changes

ilyavolodin approved these changes Oct 12, 2017

View reviewed changes

not-an-aardvark suggested changes Oct 12, 2017

View reviewed changes

majapw force-pushed the maja-add-custom-message-to-no-restricted-imports branch from d99da30 to fa9567d Compare October 12, 2017 17:34

ljharb approved these changes Oct 12, 2017

View reviewed changes

not-an-aardvark suggested changes Oct 12, 2017

View reviewed changes

majapw force-pushed the maja-add-custom-message-to-no-restricted-imports branch from fa9567d to 7de695d Compare October 12, 2017 23:17

not-an-aardvark approved these changes Oct 12, 2017

View reviewed changes

not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules labels Oct 12, 2017

platinumazure requested changes Oct 12, 2017

View reviewed changes

kaicataldo approved these changes Oct 13, 2017

View reviewed changes

majapw force-pushed the maja-add-custom-message-to-no-restricted-imports branch from 7de695d to 943964a Compare October 13, 2017 23:41

platinumazure approved these changes Oct 13, 2017

View reviewed changes

kaicataldo merged commit b02fbb6 into eslint:master Oct 14, 2017

kaicataldo mentioned this pull request Oct 14, 2017

Proposal: support custom message for all no-restricted-* rules and possibly others #8400

Closed

majapw deleted the maja-add-custom-message-to-no-restricted-imports branch October 16, 2017 15:49

eslint-deprecated bot locked and limited conversation to collaborators Apr 13, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for custom messages to restricted imports #9413

Add support for custom messages to restricted imports #9413

majapw commented Oct 12, 2017 •

edited

jsf-clabot commented Oct 12, 2017 •

edited

eslintbot commented Oct 12, 2017

eslintbot commented Oct 12, 2017

ljharb left a comment

not-an-aardvark commented Oct 12, 2017

ljharb commented Oct 12, 2017

ilyavolodin left a comment

not-an-aardvark left a comment

not-an-aardvark Oct 12, 2017

ljharb Oct 12, 2017

ljharb Oct 12, 2017

not-an-aardvark Oct 12, 2017

not-an-aardvark Oct 12, 2017

eslintbot commented Oct 12, 2017

majapw commented Oct 12, 2017

ljharb left a comment

not-an-aardvark left a comment

not-an-aardvark Oct 12, 2017

majapw Oct 12, 2017

eslintbot commented Oct 12, 2017

majapw commented Oct 12, 2017

not-an-aardvark left a comment

platinumazure left a comment

platinumazure Oct 12, 2017

platinumazure Oct 12, 2017

platinumazure Oct 12, 2017

platinumazure Oct 12, 2017

kaicataldo left a comment

kaicataldo commented Oct 13, 2017 •

edited

eslintbot commented Oct 13, 2017

majapw commented Oct 13, 2017

platinumazure left a comment

kaicataldo commented Oct 14, 2017

Add support for custom messages to restricted imports #9413

Add support for custom messages to restricted imports #9413

Conversation

majapw commented Oct 12, 2017 • edited

jsf-clabot commented Oct 12, 2017 • edited

eslintbot commented Oct 12, 2017

eslintbot commented Oct 12, 2017

ljharb left a comment

Choose a reason for hiding this comment

not-an-aardvark commented Oct 12, 2017

ljharb commented Oct 12, 2017

ilyavolodin left a comment

Choose a reason for hiding this comment

not-an-aardvark left a 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

eslintbot commented Oct 12, 2017

majapw commented Oct 12, 2017

ljharb left a comment

Choose a reason for hiding this comment

not-an-aardvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Oct 12, 2017

majapw commented Oct 12, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

platinumazure left a 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

kaicataldo left a comment

Choose a reason for hiding this comment

kaicataldo commented Oct 13, 2017 • edited

eslintbot commented Oct 13, 2017

majapw commented Oct 13, 2017

platinumazure left a comment

Choose a reason for hiding this comment

kaicataldo commented Oct 14, 2017

majapw commented Oct 12, 2017 •

edited

jsf-clabot commented Oct 12, 2017 •

edited

kaicataldo commented Oct 13, 2017 •

edited