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

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

Merged
merged 7 commits into from Sep 12, 2020

Conversation

JoshuaKGoldberg
Copy link
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 commented Jul 7, 2020

CLA assistant check
All committers have signed the CLA.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Jul 7, 2020
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features and removed triage An ESLint team member will look at this issue soon labels Aug 11, 2020
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

@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
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
Contributor Author

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

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

Choose a reason for hiding this comment

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

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 September 6, 2020 10:54
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@kaicataldo kaicataldo merged commit 3ca2700 into eslint:master Sep 12, 2020
@kaicataldo
Copy link
Member

Thanks for contributing!

@JoshuaKGoldberg JoshuaKGoldberg deleted the plugin-notice-char branch September 12, 2020 18:18
nzakas added a commit to eslint/eslintrc that referenced this pull request Jan 15, 2021
nzakas added a commit to eslint/eslintrc that referenced this pull request Jan 15, 2021
mdjermanovic pushed a commit to eslint/eslintrc that referenced this pull request 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 This issue has been archived; please open a new issue for any further discussion label Mar 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"ESLint couldn't find the plugin" notice loses a last character
5 participants