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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Corrected notice for invalid (:) plugin names #13473

Merged
merged 7 commits into from Sep 12, 2020

Conversation

JoshuaKGoldberg
Copy link
Sponsor Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Jul 7, 2020

Prerequisites checklist

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

What changes did you make? (Give an overview)

Three small, related changes:

  • The plugin name parsed in the config array factory shouldn't slice off the last digit when there isn't a / in the name.
  • Shared naming normalization shouldn't add a plugin- if the module name has a seemingly erroneous :.
  • For those : names, a new plugin-missing error message suggests that the plugin name might be invalid.

This'll be the new error contents from the example in the issue:

Oops! Something went wrong! :(

ESLint: 7.4.0

ESLint couldn't find the plugin "plugin:eslint-config-prettier".

(The package "plugin:eslint-config-prettier" was not found when loaded as a Node module from the directory "/Users/josh/repos/linttemp".)

It looks like this might not be a valid plugin name. Did you use the name of a preset configuration instead of a plugin?

The plugin "plugin:eslint-config-prettier" was referenced from the config file in ".eslintrc.js".

If you still can't figure out the problem, please stop by https://eslint.org/chat to chat with the team.

Fixes #13255. Although the issue is marked as working as intended, the comments indicate the error message given for an invalid plugin name should be improved.

Is there anything you'd like reviewers to focus on?

I'm not confident this is the right error message... would welcome someone with more ESLint context helping improve it please! 馃槃

@jsf-clabot
Copy link

@jsf-clabot jsf-clabot commented Jul 7, 2020

CLA assistant check
All committers have signed the CLA.

@eslint-deprecated eslint-deprecated bot added the triage label Jul 7, 2020
@mdjermanovic mdjermanovic added accepted bug core and removed triage labels Aug 11, 2020
Copy link
Member

@mdjermanovic mdjermanovic left a comment

@JoshuaKGoldberg thanks for the PR, and sorry for the delay!

lib/cli-engine/config-array-factory.js Outdated Show resolved Hide resolved
messages/plugin-invalid.txt Outdated Show resolved Hide resolved
messages/plugin-invalid.txt Outdated Show resolved Hide resolved
nzakas
nzakas approved these changes Aug 28, 2020
messages/plugin-invalid.txt Outdated Show resolved Hide resolved
"<%= configName %>" is invalid syntax for a config specifier.

* If your intention is to extend from a configuration exported from the plugin, add the configuration name after a slash: e.g. "<%= configName %>/myConfig".
* If this is the name of a shareable config instead of a plugin, remove the "plugin:" prefix: i.e. "<%= configName.slice("plugin:".length) %>".
Copy link
Sponsor Contributor Author

@JoshuaKGoldberg JoshuaKGoldberg Aug 29, 2020

Choose a reason for hiding this comment

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

TIL this is allowed in _.template. Confirmed locally this does what we want it to:

Oops! Something went wrong! :(

ESLint: 7.7.0

"plugin:wat" is invalid syntax for a config specifier.

* If your intention is to extend from a configuration exported from the plugin, add the configuration name after a slash: e.g. "plugin:wat/myConfig".
* If this is the name of a shareable config instead of a plugin, remove the "plugin:" prefix: i.e. "wat".

'"plugin:wat"' was referenced from the config file in "C:\Code\eslinttemp\.eslintrc.json".

If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.

Copy link
Member

@mdjermanovic mdjermanovic Sep 5, 2020

Choose a reason for hiding this comment

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

This output looks awesome!

I can also confirm locally that this works. It seems that <%- and <%= can evaluate code, although it doesn't look like that in the lodash docs.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Looks great! I left just a few small suggestions.

messages/plugin-invalid.txt Outdated Show resolved Hide resolved
messages/plugin-invalid.txt Outdated Show resolved Hide resolved
messages/plugin-invalid.txt Outdated Show resolved Hide resolved
messages/plugin-invalid.txt Outdated Show resolved Hide resolved
tests/lib/cli-engine/config-array-factory.js Outdated Show resolved Hide resolved
tests/lib/cli-engine/config-array-factory.js Outdated Show resolved Hide resolved
JoshuaKGoldberg and others added 2 commits Sep 6, 2020
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Looks great, thanks!

@mdjermanovic mdjermanovic requested a review from nzakas Sep 6, 2020
nzakas
nzakas approved these changes Sep 9, 2020
@kaicataldo kaicataldo merged commit 3ca2700 into eslint:master Sep 12, 2020
12 checks passed
@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Sep 12, 2020

Thanks for contributing!

@JoshuaKGoldberg JoshuaKGoldberg deleted the plugin-notice-char branch Sep 12, 2020
nzakas added a commit to eslint/eslintrc that referenced this issue Jan 15, 2021
nzakas added a commit to eslint/eslintrc that referenced this issue Jan 15, 2021
mdjermanovic pushed a commit to eslint/eslintrc that referenced this issue Jan 15, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Mar 18, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age label Mar 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted archived due to age bug core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants