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

feat: Support plugins with scoped names #104

Merged
merged 2 commits into from Jun 9, 2016
Merged

feat: Support plugins with scoped names #104

merged 2 commits into from Jun 9, 2016

Conversation

@scottnonnenberg
Copy link
Contributor

@scottnonnenberg scottnonnenberg commented Jun 8, 2016

I was getting this error:

Error: Cannot find module 'eslint-plugin-@scottnonnenberg/thehelp'
    at Function.Module._resolveFilename (module.js:325:15)
    at Function.Module._load (module.js:276:25)
    at Module.require (module.js:353:17)
    at require (internal/module.js:12:17)
    at getPluginRule (node_modules/eslint-find-rules/src/lib/rule-finder.js:41:26)
    ...
@codecov-io
Copy link

@codecov-io codecov-io commented Jun 8, 2016

Current coverage is 100%

Merging #104 into master will not change coverage

@@           master   #104   diff @@
====================================
  Files           9      9          
  Lines         199    206     +7   
  Methods         0      0          
  Messages        0      0          
  Branches        0      0          
====================================
+ Hits          199    206     +7   
  Misses          0      0          
  Partials        0      0          

Powered by Codecov. Last updated by 851d6e3...878a7f1

'react/bar-rule',
'react/baz-rule',
'react/foo-rule',
'something/two-rule',

This comment has been minimized.

@sarbbottam

sarbbottam Jun 8, 2016
Owner

Should something/two-rule to be @someone/something/two-rule?

This comment has been minimized.

@ta2edchimp

ta2edchimp Jun 8, 2016
Collaborator

As I understand the ESLint docs, the rules are to be referenced by their containing plugin's name (regardless of their possibly scoped package's name).

So for eslint-plugin-<plugin-name>'s rules, as well as @scope/eslint-plugin-<plugin-name>, its rules should be configured via <plugin-name>/<rule-name>.

Please correct me, if I'm wrong.

This comment has been minimized.

@sarbbottam

sarbbottam Jun 8, 2016
Owner

May not be a practical use case, curious to know, how does eslint handle, two or more scoped plugins, of the same name?

This comment has been minimized.

@scottnonnenberg

scottnonnenberg Jun 8, 2016
Author Contributor

@ta2edchimp I tested it in another project as I was working on this, and yes, for configuration rules take only the plugin name and not the scope name. Wasn't my first guess.


return name
}

This comment has been minimized.

@sarbbottam

sarbbottam Jun 8, 2016
Owner

_pluginNameToModule and _pluginNameToRulePrefix are almost same, other than the return value, could you please combine them in to one?

Something like so ...

function _normalizePluginName(name) {
  var scopedRegex = /(@[^/]+)\/(.+)/
  var match = scopedRegex.exec(name)

  if (match) {
    return {
      module: match[1] + '/' + 'eslint-plugin-' + match[2],
      prefix: match[2]
    }
  }

  return {
    module: 'eslint-plugin-' + name
    prefix: name
  }
}
var pluginConfig = require('eslint-plugin-' + plugin)
var module = _pluginNameToModule(plugin)
var prefix = _pluginNameToRulePrefix(plugin)
var pluginConfig = require(module)

This comment has been minimized.

@sarbbottam

sarbbottam Jun 8, 2016
Owner

... and make corresponding updates

'@someone/eslint-plugin-something': {
rules: {
'one-rule': true,
'two-rule': true,

This comment has been minimized.

@sarbbottam

sarbbottam Jun 8, 2016
Owner

I would stick to foo, bar rules for consistency.
And even modified the identifier names from react to plugin and something to scoped-plugin.

When we revisit the code later on, the names will be self explanatory.

@sarbbottam
Copy link
Owner

@sarbbottam sarbbottam commented Jun 8, 2016

@scottnonnenberg Thanks for the PR!

@scottnonnenberg
Copy link
Contributor Author

@scottnonnenberg scottnonnenberg commented Jun 8, 2016

That should take care of all the feedback - let me know if you've got anything else!

@sarbbottam
Copy link
Owner

@sarbbottam sarbbottam commented Jun 8, 2016

👍 looks great! Thanks!

@ta2edchimp please have a look and merge if you don't have any comments.

@sarbbottam
Copy link
Owner

@sarbbottam sarbbottam commented Jun 8, 2016

Ohh, @scottnonnenberg please add yourself to the contributors list

@ta2edchimp ta2edchimp merged commit 2443474 into sarbbottam:master Jun 9, 2016
2 checks passed
2 checks passed
codecov/project 100% (+0.00%) compared to 851d6e3
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ta2edchimp
Copy link
Collaborator

@ta2edchimp ta2edchimp commented Jun 9, 2016

Really cool, thanks for the PR!

scottnonnenberg added a commit to scottnonnenberg/eslint-config-thehelp that referenced this pull request Jun 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants