Support overridable configs #58

jamestalmage · 2015-12-14T06:54:31Z

Fixes #22.

This allows for overridable configs based on glob patterns.

Usage is as follows:

{
  "xo": {
    "esnext": false,
    "overrides": [
      {
        "files": "test/*.js",
        "esnext": true
      },
      {
         "files": "test/foo.js",
         "space": 3
      }
    ]
  }
}

The base config is:

{
  "esnext": false
}

For everything in the test directory the config is:

{
  "esnext": true
}

With the exception of test/foo.js which has the following config:

{
  "esnext": true,
  "space": 3
}

It is important to note that overrides are cumulative, and processed in the order specified in the array.
If two overrides match the same file, the latter will takes precedence over the former.

jamestalmage · 2015-12-14T06:58:47Z

We had discussed using object hashes for overrides:

"overrides": {
  "**/foo.js": { "esnext": false } 
}

I decided against doing it that way, because it gives you no control over merge order if two patterns match the same file.

jamestalmage · 2015-12-14T07:00:35Z

There is still debate over exactly what to name some of these options. Once that is settled, I will update this PR (including docs).

SamVerschueren · 2015-12-14T19:17:34Z

The overrides functionality is really nice! Is it possible to provide a concrete example on the benefit of the array versus the object hashes? It seems like it just makes everything more complex because the order in the array is important. It's because of merge conflicts when more globbing patterns matches the same file, right? But why not apply the most specific rules for that file?

"overrides": {
  "test/test.js": {"esnext": true},
  "test/*.js": { "esnext": false } 
}

Because test/test.js is more specific, the esnext rule should be true for the file test/test.js. Independent of the order of the keys.

Just wanted some more information regarding this :).

jamestalmage · 2015-12-14T19:42:51Z

It's because of merge conflicts when more globbing patterns matches the same file, right?

Yes. The array syntax is necessary to support multiple glob patterns anyways:

"overrides": [
  {
    "files": ["test/*.js", "!test/foo.js"]
  }
]

So I think the array syntax needs to be a part of the final solution no matter what. It might be possible to allow both array and object.

To be honest, the array solution was just easier to implement.

But why not apply the most specific rules for that file?

I am not sure how to reliably determine "the most specific match". Which of the following more specifically matches test/foo.js?

**/foo.js
test/*oo.js

Also, how do we then merge settings? Least specific to most specific? In that case the following is confusing:

"overrides": {
  "test/test.js": {"esnext": true},
  "test/*.js": { "esnext": false, spaces:3} 
}

If we merge every match, in order of specificity., the settings for test/test.js are obtained by merging the least specific ({esnext: false, spaces: 3}), followed by the most specific ({esnext:true}). But that's opposite the order I have them listed above. That is confusing.

I think if we allow the object pattern we should only apply a single "most specific" match (I am still not sure how best to determine that).

SamVerschueren · 2015-12-14T19:50:52Z

The array syntax is necessary to support multiple glob patterns anyways

Good point

I am not sure how to reliably determine "the most specific match"

Not sure either

But that's opposite the order I have them listed above. That is confusing.

I don't think that's confusing. But maybe that's just me.

In general, whatever gets implemented, as long as it's clearly documented, we're good :). Just wanted to share my opinion. Hopefully others will pick in into this discussion.

jamestalmage · 2015-12-14T19:54:55Z

Yeah, I fully intend to document before it gets merged. Still looking for feedback at this point.

jamestalmage · 2015-12-15T00:52:38Z

options-manager.js

+		// always use the Babel parser so it won't throw
+		// on esnext features in normal mode
+		config.parser = 'babel-eslint';
+		config.plugins = ['babel'];


Also, I noticed that if opts.esnext is not true, we overwrite any plugins they might have specified. Should we just be appending/prepending babel to the list?

Oh, noes. That's why 90c95ed#commitcomment-14948953 Good eye! :)

It should be prepended.

@sindresorhus You want me to fix it here or a separate PR from master?

@jamestalmage Master. I need to do a patch release. :)

If the `esnext` option is `false`, we currently blow away existing plugins, replacing it with only the `babel` plugin. This prepends it. This does not come with a test, as option handling is currently pretty difficult. We can add the appropriate test for this there. Reference: xojs#58 (comment)

jamestalmage · 2015-12-22T04:06:45Z

@sindresorhus @kevva @vdemedes - Do you guys have any additional feedback? If I get this rebased is it good to merge?

vadimdemedes · 2015-12-22T07:35:26Z

Good work, looking forward to using this!

sindresorhus · 2015-12-26T15:23:08Z

options-manager.js

+var resolveFrom = require('resolve-from');
+var objectAssign = require('object-assign');
+var homeOrTmp = require('home-or-tmp');
+var mutlimatch = require('multimatch');


mutlimatch => multimatch

sindresorhus · 2015-12-26T15:53:36Z

Sorry about the delay. I've just had too many PRs to review. Took a thorough look now and it generally looks fantastic! Also thanks for refactoring the code into a more readable and maintainable state 👌.

I added a few minor inline comments, but it's pretty close to be mergeable. Currently, all the individual pieces are tested, but could use a real world integration test in test.js with a fixture and a real package.json, just to ensure all the "glue" works correctly. Also needs docs, but I would be happy to write that if you don't want / have time.

It's 🎄so no rush on this :)

jamestalmage · 2015-12-27T06:10:31Z

@sindresorhus - this should be all set.

I added an undocumented --no-local flag to disable the local check. Otherwise we were getting our old version for all the tests in test/cli.js

jamestalmage · 2015-12-28T03:23:09Z

@sindresorhus - Are we good to merge here? I would like to avoid rebasing again.

Once merged I suggest we let it live on master and test it against a few projects before release. It was a significant refactor followed by a problematic rebase. I'm fairly confident I got it right, but better to practice caution.

sindresorhus · 2015-12-29T01:17:52Z

Yes, landed :D I'm super happy about how this turned out.

sindresorhus · 2015-12-29T01:18:38Z

Gonna let it live on master for a few days while I test it on some of my projects.

dvdln · 2016-01-26T15:38:27Z

...any chance this will be released anytime soon?

sindresorhus · 2016-01-26T16:13:05Z

@dvdln When this issue is resolved: #71

jamestalmage reviewed Dec 15, 2015
View reviewed changes

jamestalmage mentioned this pull request Dec 15, 2015

Don't destroy plugins. #59

Merged

sindresorhus reviewed Dec 26, 2015
View reviewed changes

jamestalmage added 8 commits December 26, 2015 23:43

refactor opts processing

04dc160

support overrides configs

f4e5493

make the groupConfig test a little more complex

c08cb40

redo changes from xojs#62, which were wiped out in the rebase.

f2caf92

fix more regressions from a bad rebase

127678a

incorporate PR feedback

057c69d

add documentation for Overrides

9d915d4

add test fixture

34924f8

jamestalmage force-pushed the refactor-opts-processing branch from 1cfad64 to 34924f8 Compare December 27, 2015 06:02

jamestalmage added 3 commits December 27, 2015 01:11

add --no-local flag to the other cli test

4257218

better language in the readme

53bcbd2

more doc improvements

6f86a98

sindresorhus changed the title ~~Support overridable configs. Fixes #22~~ Support overridable configs Dec 29, 2015

jamestalmage closed this in 3cf0ccf Dec 29, 2015

SamVerschueren mentioned this pull request Jan 8, 2016

Support file-type-specific settings microsoft/vscode#1587

Closed

fregante mentioned this pull request Mar 4, 2017

Unable to customize certain rules via shareable config #71

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support overridable configs #58

Support overridable configs #58

jamestalmage commented Dec 14, 2015

jamestalmage commented Dec 14, 2015

jamestalmage commented Dec 14, 2015

SamVerschueren commented Dec 14, 2015

jamestalmage commented Dec 14, 2015

SamVerschueren commented Dec 14, 2015

jamestalmage commented Dec 14, 2015

jamestalmage Dec 15, 2015

sindresorhus Dec 15, 2015

jamestalmage Dec 15, 2015

sindresorhus Dec 15, 2015

jamestalmage commented Dec 22, 2015

vadimdemedes commented Dec 22, 2015

sindresorhus Dec 26, 2015

sindresorhus commented Dec 26, 2015

jamestalmage commented Dec 27, 2015

jamestalmage commented Dec 28, 2015

sindresorhus commented Dec 29, 2015

sindresorhus commented Dec 29, 2015

dvdln commented Jan 26, 2016

sindresorhus commented Jan 26, 2016

Support overridable configs #58

Support overridable configs #58

Conversation

jamestalmage commented Dec 14, 2015

jamestalmage commented Dec 14, 2015

jamestalmage commented Dec 14, 2015

SamVerschueren commented Dec 14, 2015

jamestalmage commented Dec 14, 2015

SamVerschueren commented Dec 14, 2015

jamestalmage commented Dec 14, 2015

jamestalmage Dec 15, 2015

Choose a reason for hiding this comment

sindresorhus Dec 15, 2015

Choose a reason for hiding this comment

jamestalmage Dec 15, 2015

Choose a reason for hiding this comment

sindresorhus Dec 15, 2015

Choose a reason for hiding this comment

jamestalmage commented Dec 22, 2015

vadimdemedes commented Dec 22, 2015

sindresorhus Dec 26, 2015

Choose a reason for hiding this comment

sindresorhus commented Dec 26, 2015

jamestalmage commented Dec 27, 2015

jamestalmage commented Dec 28, 2015

sindresorhus commented Dec 29, 2015

sindresorhus commented Dec 29, 2015

dvdln commented Jan 26, 2016

sindresorhus commented Jan 26, 2016