Update: Throw error if whitespace found in plugin name (fixes #6854) #6960

Merged
merged 1 commit into from Sep 3, 2016

Projects

None yet

9 participants

@jostrander
Contributor

What issue does this pull request address?

Issue #6854

What changes did you make? (Give an overview)

Added a check to see if white-space is in the plugin name before requiring plugins and throw an error if white-space is found. Also added test.

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

Not exactly sure on the wording of the error messages. Will update if needed.

@mention-bot

@jostrander, thanks for your PR! By analyzing the annotation information on this pull request, we identified @nzakas, @mysticatea and @kaicataldo to be potential reviewers

@eslintbot

LGTM

@jquerybot

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.


If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

@eslintbot

LGTM

@jostrander
Contributor

Updated author info to match CLA

@eslintbot

LGTM

@jquerybot jquerybot added CLA: Valid and removed CLA: Error labels Aug 22, 2016
@platinumazure
Member
platinumazure commented Aug 22, 2016 edited

EDIT: I'm okay with just doing whitespace here (as long as it's space/tab/newline via regex).


Thanks so much for starting this work!

My only question is whether we should consider broadening this very slightly to check that the plugin name matches NPM's package name validation, and then just report that the package name is invalid, without restricting ourselves to whitespace. But please don't make any changes just because I'm babbling here 😄 Just think about it and let's see if anyone else on the team agrees. Thanks!

@kaicataldo
Member

@platinumazure I like that suggestion - checking for spaces only feels a little arbitrary to me

@platinumazure
Member
platinumazure commented Aug 23, 2016 edited

EDIT: I'm okay with just doing whitespace here (as long as it's space/tab/newline via regex).


Okay, I've raised the point on the issue (here). If we want to accept the whitespace check for now, I'm okay with that. I just wanted to make sure it was discussed.

@nzakas nzakas and 2 others commented on an outdated diff Aug 23, 2016
lib/config/plugins.js
@@ -108,6 +108,16 @@ module.exports = {
pluginNameWithoutPrefix = removePrefix(pluginNameWithoutNamespace);
let plugin = null;
+ if (pluginName.indexOf(" ") >= 0) {
@nzakas
nzakas Aug 23, 2016 Member

This should be a regex check because there could also be a tab character or newline character, for example.

@jostrander
jostrander Aug 23, 2016 Contributor

Good catch, I'll update.

@kaicataldo
kaicataldo Aug 23, 2016 Member

👍 This alleviates my concern in my comment above

@nzakas
Member
nzakas commented Aug 23, 2016

It depends on how easy it is to spot the problem. I think the concern here is that whitespace is hard to spot, so it makes sense to call it out as a specific error. If you just throw up a "plugin name validation error", it might not be obvious what the problem is.

@platinumazure
Member

Okay, that's fair enough. If we can just get the regex space detection in per @nzakas' request via inline comment, then I'll withdraw my other question.

@jostrander
Contributor

Sounds good, will rebase and update.

@eslintbot

LGTM

@jostrander
Contributor
jostrander commented Aug 23, 2016 edited

Not sure what I broke here. I soft reset and recommitted then rebased and pushed and it's all errored out.


Nevermind, rebase was out of date.

@eslintbot

LGTM

@mysticatea mysticatea commented on an outdated diff Aug 23, 2016
lib/config/plugins.js
@@ -114,6 +114,16 @@ module.exports = {
longName = pluginNamespace + PLUGIN_NAME_PREFIX + pluginNameWithoutPrefix;
let plugin = null;
+ if (pluginName.match(/\s+/)) {
+ const whitespaceError = new Error("Whitespace found in plugin name eslint-plugin-" + pluginNameWithoutPrefix);
+
+ whitespaceError.messageTemplate = "whitespace-found";
+ whitespaceError.messageData = {
+ pluginName: pluginNameWithoutPrefix
@mysticatea
mysticatea Aug 23, 2016 Member

Could you use longName in error messages? Package names might have scope qualifiers (e.g. @mysticatea/eslint-plugin-test)
Related to #6939

@vitorbal vitorbal commented on an outdated diff Aug 24, 2016
tests/lib/config/plugins.js
@@ -85,6 +85,12 @@ describe("Plugins", function() {
assert.deepEqual(Rules.get("example/qux"), plugin.rules.qux);
});
+ it("should throw an error when a plugin has whitespace", function() {
+ assert.throws(function() {
+ StubbedPlugins.load("whitespace ");
+ }, /Whitespace found in plugin name/);
+ });
+
@vitorbal
vitorbal Aug 24, 2016 Member

Should we have some tests for plugin names with tab characters or newline characters as well?

@platinumazure
Member

@jostrander By my count, just two small things to finish up. Anything we can do to help you finish this?

@eslintbot

LGTM

@platinumazure platinumazure and 2 others commented on an outdated diff Aug 29, 2016
messages/whitespace-found.txt
@@ -0,0 +1,3 @@
+ESLint couldn't find the plugin "eslint-plugin-<%- pluginName %>". It looks like there is whitespace in the plugin name.
@platinumazure
platinumazure Aug 29, 2016 Member

Now that the longName is being used in the messageData, I think this should just be ESLint couldn't find the plugin "<%- longName %>". @mysticatea can you confirm as well please?

@jostrander
jostrander Aug 29, 2016 Contributor

Ah yeah I knew I had it the other way for a reason. You're probably right, but I'll wait for @mysticatea to confirm.

@mysticatea
mysticatea Aug 29, 2016 Member

Yes, longName includes eslint-plugin- part.
This commit also removed the part.

@platinumazure
Member

@jostrander These changes look great! Just one follow-up in the message template as a result of the messageData change requested by mysticatea, then I think this might be ready to land!

(Note: We might take another day or two to merge because there's the possibility of a patch release on Monday)

@mysticatea mysticatea commented on an outdated diff Aug 29, 2016
lib/config/plugins.js
@@ -114,6 +114,16 @@ module.exports = {
longName = pluginNamespace + PLUGIN_NAME_PREFIX + pluginNameWithoutPrefix;
let plugin = null;
+ if (pluginName.match(/\s+/)) {
+ const whitespaceError = new Error("Whitespace found in plugin name eslint-plugin-" + pluginNameWithoutPrefix);
@mysticatea
mysticatea Aug 29, 2016 Member

This is same.

@nzakas
Member
nzakas commented Sep 2, 2016

@jostrander are you going to finish this up?

@jostrander
Contributor

Ah, yep, give me a few minutes and I'll push that change up.

@eslintbot

LGTM

@vitorbal
Member
vitorbal commented Sep 2, 2016

Thanks, @jostrander! Looks like you missed this last comment by @mysticatea: #6960 (comment)
Besides that, LGTM

@eslintbot

LGTM

@nzakas nzakas commented on an outdated diff Sep 3, 2016
lib/config/plugins.js
@@ -114,6 +114,16 @@ module.exports = {
longName = pluginNamespace + PLUGIN_NAME_PREFIX + pluginNameWithoutPrefix;
let plugin = null;
+ if (pluginName.match(/\s+/)) {
+ const whitespaceError = new Error("Whitespace found in plugin name " + pluginName);
@nzakas
nzakas Sep 3, 2016 Member

One last thing here, can you surround the plugin name with single quotes?

@nzakas nzakas commented on an outdated diff Sep 3, 2016
tests/lib/config/plugins.js
@@ -85,6 +85,21 @@ describe("Plugins", function() {
assert.deepEqual(Rules.get("example/qux"), plugin.rules.qux);
});
+ it("should throw an error when a plugin has whitespace", function() {
+ assert.throws(function() {
+ StubbedPlugins.load("whitespace ");
+ }, /Whitespace found in plugin name/);
@nzakas
nzakas Sep 3, 2016 Member

Can you test the full message to show that the plugin name is included?

@nzakas nzakas commented on an outdated diff Sep 3, 2016
messages/whitespace-found.txt
@@ -0,0 +1,3 @@
+ESLint couldn't find the plugin "<%- pluginName %>". It looks like there is whitespace in the plugin name.
@nzakas
nzakas Sep 3, 2016 Member

I think we can be a bit more instructive here. Maybe:

ESLint couldn't load the plugin "<%- pluginName %>" because there is whitespace in the name. Please check your configuration and remove all whitespace from the plugin name.

@jostrander jostrander Update: Throw error if whitespace found in plugin name (fixes #6854)
e1cd310
@eslintbot

LGTM

@nzakas
Member
nzakas commented Sep 3, 2016

Lgtm. Thanks for contributing!

@nzakas nzakas merged commit be29599 into eslint:master Sep 3, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jQuery Foundation CLA All authors have signed the CLA
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment