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

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

Merged
merged 1 commit into from Sep 3, 2016

Conversation

jostrander
Copy link
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
Copy link

@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
Copy link

LGTM

@jquerybot
Copy link

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
Copy link

LGTM

@jostrander
Copy link
Contributor Author

Updated author info to match CLA

@eslintbot
Copy link

LGTM

@platinumazure
Copy link
Member

platinumazure commented Aug 22, 2016

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
Copy link
Member

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

@platinumazure
Copy link
Member

platinumazure commented Aug 23, 2016

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.

@@ -108,6 +108,16 @@ module.exports = {
pluginNameWithoutPrefix = removePrefix(pluginNameWithoutNamespace);
let plugin = null;

if (pluginName.indexOf(" ") >= 0) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I'll update.

Copy link
Member

Choose a reason for hiding this comment

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

👍 This alleviates my concern in my comment above

@nzakas
Copy link
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
Copy link
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
Copy link
Contributor Author

Sounds good, will rebase and update.

@eslintbot
Copy link

LGTM

@jostrander
Copy link
Contributor Author

jostrander commented Aug 23, 2016

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
Copy link

LGTM


whitespaceError.messageTemplate = "whitespace-found";
whitespaceError.messageData = {
pluginName: pluginNameWithoutPrefix
Copy link
Member

Choose a reason for hiding this comment

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

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

@platinumazure
Copy link
Member

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

@eslintbot
Copy link

LGTM

@@ -0,0 +1,3 @@
ESLint couldn't find the plugin "eslint-plugin-<%- pluginName %>". It looks like there is whitespace in the plugin name.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

@platinumazure
Copy link
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)

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

This is same.

@nzakas
Copy link
Member

nzakas commented Sep 2, 2016

@jostrander are you going to finish this up?

@jostrander
Copy link
Contributor Author

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

@eslintbot
Copy link

LGTM

@vitorbal
Copy link
Member

vitorbal commented Sep 2, 2016

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

@eslintbot
Copy link

LGTM

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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

@eslintbot
Copy link

LGTM

@nzakas
Copy link
Member

nzakas commented Sep 3, 2016

Lgtm. Thanks for contributing!

@nzakas nzakas merged commit be29599 into eslint:master Sep 3, 2016
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants