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
Conversation
@jostrander, thanks for your PR! By analyzing the annotation information on this pull request, we identified @nzakas, @mysticatea and @kaicataldo to be potential reviewers |
LGTM |
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. |
LGTM |
Updated author info to match CLA |
LGTM |
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! |
@platinumazure I like that suggestion - checking for spaces only feels a little arbitrary to me |
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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. |
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. |
Sounds good, will rebase and update. |
LGTM |
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. |
LGTM |
|
||
whitespaceError.messageTemplate = "whitespace-found"; | ||
whitespaceError.messageData = { | ||
pluginName: pluginNameWithoutPrefix |
There was a problem hiding this comment.
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
@jostrander By my count, just two small things to finish up. Anything we can do to help you finish this? |
LGTM |
@@ -0,0 +1,3 @@ | |||
ESLint couldn't find the plugin "eslint-plugin-<%- pluginName %>". It looks like there is whitespace in the plugin name. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is same.
@jostrander are you going to finish this up? |
Ah, yep, give me a few minutes and I'll push that change up. |
1affe88
to
fef7a9f
Compare
LGTM |
Thanks, @jostrander! Looks like you missed this last comment by @mysticatea: #6960 (comment) |
fef7a9f
to
cbb748c
Compare
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); |
There was a problem hiding this comment.
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?
cbb748c
to
e1cd310
Compare
LGTM |
Lgtm. Thanks for contributing! |
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.