Refactor

[sarbbottam]

The binary interface has not been changed. However if one would prefer to require the module, below exposed APIs are available.

• getCurrentRules // get all the current rules instead of referring the extended files or documentation
• getPluginRules // get all the plugin rules instead of referring the extended files or documentation
• getAllAvailableRules // get all the availale rules instead of referring eslint and pluging packages or documentation
• getNewRules

I look forward to your concerns. 🐰 Happy Easter! 🐰

[codecov-io]

Current coverage is 100.00%

Merging #27 into master will not affect coverage as of 94cf547

@@            master     #27   diff @@
======================================
  Files            2       1     -1
  Stmts           43      48     +5
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit             43      48     +5
  Partial          0       0       
  Missed           0       0       

Review entire Coverage Diff as of 94cf547

Powered by Codecov. Updated on successful CI builds.

[ta2edchimp]

Personally, I'm all for exposing a module's method to be used as a lib 👍

But to be honest, I would have rather just git mv ./bin-utils.js ./lib/rule-finder.js, included ./index.js's content into ./lib/rule-finder.js and adjusted package.json's main field.
I like the simple functional approach the methods utilize as of now, as well as that we expose the compiled config via getConfig() method.

So, I'm really unsure whether to refactor it exactly this way, and I tend towards declining it.
May @kentcdodds decide ...

[sarbbottam]

@ta2edchimp thanks for sharing your concerns.

I like the simple functional approach the methods

Me too, the proposed code is pseudo-functional/pseudo-object-orieneted, if I may say so. All the computation is purely functional. It does not depends on the state.

var configFile = _getConfigFile(specifiedFile)
var config = _getConfig(configFile)
var currentRules = _getCurrentRules(config)
var pluginRules = _getPluginRules(config)
var allRules = _getAllAvailableRules(pluginRules)
var newRules = difference(allRules, currentRules)

In fact, its similar to bin-utils combined with index along with following lines from bin

var config = binUtils.getConfig(process.argv[2])
var currentRules = binUtils.getCurrentRules(config)
var pluginRules = binUtils.getPluginRules(config)

The only reason to wrap the above stated computation in constructor is encapsulation.

The only drawback I see in the proposed approach is two calls.

var ruleFinder = new RuleFinder(specifiedFile)
var newRules = ruleFinder.getNewRules()

If any changes takes place between those two statement, for example, eslint config is updated after var ruleFinder = new RuleFinder(specifiedFile), it would not take the changes in the account and ruleFinder = new RuleFinder(specifiedFile) needs to be re-executed.

But it would be a rare scenario.
Even in the current approach, one need to invoke more than one function and same could happen.

var config = binUtils.getConfig(process.argv[2])
var currentRules = binUtils.getCurrentRules(config)
var pluginRules = binUtils.getPluginRules(config)

However we could stick to pure functional and always accept the config file as the argument. But I guess the APIs need to be modified then.

[ta2edchimp]

Well I actually think of this approach to not take changes into account as a real advantage!
I could imagine scenarios, where you could use this to create diffs / track changes between different versions of a config file or sth. like that, when using this module as a lib.

Well, could we at least think of letting the module expose a factory function to return the desired RuleFinder object?
_{It's simply that I personally ... to keep it very brief ... don't like any appearances of the keyword new.}

tl;dr, sth like this (shortened heavily):

function getRuleFinder(specifiedFile) {
  // the private stuff ...

  return {
    // prop shorthand notation only for demo purposes
    getCurrentRules() {
      return currentRules
    },
    getPluginRules() {
      return pluginRules
    },
    getAllAvailableRules() {
      return allRules
    },
    getNewRules() {
      return newRules
    }
  }
}

module.exports = getRuleFinder

I also think about whether something like getUnusedRules or findUnconfiguredRules would be a better and more concise name instead of getNewRules (as, you know, they don't have to necessarily be new, but are rather unconfigured / unused).

What do you think?

PS: Sorry if I may sound a bit ranting at times, it is absolutely not intended, but as not being a native speaker, it's just hard sometimes to express exactly what I mean 😜

[sarbbottam]

It's simply that I personally ... to keep it very brief ... don't like any appearances of the keyword new.

Me neither, for ES2015 class, we have to use new and thus got back to the convention

I also think about whether something like getUnusedRules or findUnconfiguredRules would be a better and more concise name instead of getNewRules (as, you know, they don't have to necessarily be new, but are rather unconfigured / unused).

👍 Totally, I am bad with, naming things.

Well, could we at least think of letting the module expose a factory function to return the desired RuleFinder object?

function getRuleFinder(specifiedFile) {
  ...
  return {
    ...
  }
}
module.exports = getRuleFinder

How about?

module.exports = function(specifiedFile) {
  return new RuleFinder(specifiedFile)
}

[ta2edchimp]

I'd be fine with that!

Just for the sake of completeness, I'd do

module.exports = function getRuleFinder(specifiedFile) {
  return new RuleFinder(specifiedFile)
}

or ... = function ruleFinder(...) or the like . . .

[sarbbottam]

@ta2edchimp could you take another look?

[ta2edchimp]

Will have a thorough look tomorrow (only on the phone atm), but looks good to me as far as I can tell now, and will probably going to merge, when @kentcdodds has nothing to add 😃

[ta2edchimp]

Some things that we have to consider prior to merging:

1. This introduces a Breaking Change (exposed API and its usage have changed).
   • Relevant commit messages should therefore be adjusted accordingly.
2. Documentation should reflect this change (that is, usage as a lib as a whole).

Had to track this here to not forget until tomorrow as I'm almost 😴

[sarbbottam]

@ta2edchimp Thanks for pointing out.

Some things that we have to consider prior to merging:

• updated the commit message to add BREAKING CHANGE ea617cf
• updated the usage documentation

Please have look, and let me know, if any changes required. Thanks!!

[kentcdodds]

LGTM

[ta2edchimp]

LGTM too.
Did a dry run on semantic-release and it works as I hoped.

👍