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 support for custom messages to restricted imports #9413

Merged
merged 1 commit into from Oct 14, 2017

Conversation

@majapw
Contributor

majapw commented Oct 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jsf-clabot

jsf-clabot Oct 12, 2017

CLA assistant check
All committers have signed the CLA.

jsf-clabot commented Oct 12, 2017

CLA assistant check
All committers have signed the CLA.

@eslintbot

This comment has been minimized.

Show comment
Hide comment
@eslintbot

eslintbot Oct 12, 2017

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 commented Oct 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@eslintbot

eslintbot commented Oct 12, 2017

LGTM

@ljharb

ljharb approved these changes Oct 12, 2017

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

@not-an-aardvark

This comment has been minimized.

Show comment
Hide comment
@not-an-aardvark

not-an-aardvark Oct 12, 2017

Member

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.

Member

not-an-aardvark commented Oct 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Oct 12, 2017

Contributor

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.

Contributor

ljharb commented Oct 12, 2017

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.

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

This comment has been minimized.

@not-an-aardvark

not-an-aardvark Oct 12, 2017

Member

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

@not-an-aardvark

not-an-aardvark Oct 12, 2017

Member

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

This comment has been minimized.

@ljharb

ljharb Oct 12, 2017

Contributor

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

@ljharb

ljharb Oct 12, 2017

Contributor

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

This comment has been minimized.

@ljharb

ljharb Oct 12, 2017

Contributor

(specifically, by adding minLength: 1)

@ljharb

ljharb Oct 12, 2017

Contributor

(specifically, by adding minLength: 1)

This comment has been minimized.

@not-an-aardvark

not-an-aardvark Oct 12, 2017

Member

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

not-an-aardvark Oct 12, 2017

Member

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

Show outdated Hide outdated lib/rules/no-restricted-imports.js
@eslintbot

This comment has been minimized.

Show comment
Hide comment
@eslintbot

eslintbot commented Oct 12, 2017

LGTM

@majapw

This comment has been minimized.

Show comment
Hide comment
@majapw

majapw Oct 12, 2017

Contributor

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

Contributor

majapw commented Oct 12, 2017

@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

ljharb approved these changes Oct 12, 2017

(ノ◕ヮ◕)ノ*:・゚✧

@not-an-aardvark

Looks good aside from a typo in the documentation.

Show outdated Hide outdated docs/rules/no-restricted-modules.md
@eslintbot

This comment has been minimized.

Show comment
Hide comment
@eslintbot

eslintbot commented Oct 12, 2017

LGTM

@majapw

This comment has been minimized.

Show comment
Hide comment
@majapw

majapw Oct 12, 2017

Contributor

@not-an-aardvark addressed!

Contributor

majapw commented Oct 12, 2017

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

Show outdated Hide outdated docs/rules/no-restricted-imports.md
Show outdated Hide outdated docs/rules/no-restricted-modules.md
Show outdated Hide outdated lib/rules/no-restricted-imports.js
Show outdated Hide outdated lib/rules/no-restricted-modules.js
@kaicataldo

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

@kaicataldo

This comment has been minimized.

Show comment
Hide comment
@kaicataldo

kaicataldo Oct 13, 2017

Member

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.

Member

kaicataldo commented Oct 13, 2017

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.

Update: Add support for custom messages to restricted imports
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

This comment has been minimized.

Show comment
Hide comment
@eslintbot

eslintbot commented Oct 13, 2017

LGTM

@majapw

This comment has been minimized.

Show comment
Hide comment
@majapw

majapw Oct 13, 2017

Contributor

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

Contributor

majapw commented Oct 13, 2017

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

@platinumazure

LGTM, thanks for contributing!

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@kaicataldo

This comment has been minimized.

Show comment
Hide comment
@kaicataldo

kaicataldo Oct 14, 2017

Member

Thanks for contributing to ESLint!

Member

kaicataldo commented Oct 14, 2017

Thanks for contributing to ESLint!

@majapw majapw deleted the majapw:maja-add-custom-message-to-no-restricted-imports branch Oct 16, 2017

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.