Organize config options #1042

IanVS · 2017-10-30T04:39:59Z

This is an attempt to introduce some organization into our config settings. Currently, they are ordered alphabetically by config name, which is to say, they're in completely random order. That's pretty confusing for new users, trying to understand our 15 different options.

In an attempt to organize them and add some clarity, I've broken out three new sections, and grouped the remaining at the top (with boolean options above text options, because I think that looks nicer).

The groups are:

Global (global ESLint settings)
Disabling (ways in which ESLint warnings can be turned off)
Advanced (less common options that nearly all users can leave at the defaults)

The method of grouping settings is a little unfortunate, because it extends down into our code and specs. I believe that I have made the necessary changes, but they should be reviewed. I recommend searching the codebase for the names of each setting that is being grouped, and verifying that they are being correctly referenced.

If this PR is accepted, I will open another PR updating our README with some more in-depth explanations of the groupings and settings themselves.

IanVS · 2017-10-30T04:41:53Z

I'd love for @skylize to take a look at this as well.

skylize · 2017-10-30T13:33:51Z

I like this on first glance. I'll take a deeper look and get back to you.

Arcanemagus

Overall I really like this idea, there are just a few things I spotted that need some tweaking 😉.

Arcanemagus · 2017-10-30T16:27:47Z

README.md

@@ -53,8 +53,7 @@ eslint --init
 ```

 Alternatively you can create the `.eslintrc` file by yourself. It is a good
-idea to have a look at the [Get Started With ESLint](http://devnull.guru/get-started-with-eslint/)
-blog post by [IanVS](https://github.com/IanVS) and [the ESLint documentation](http://eslint.org/docs/user-guide/configuring),
+idea to have a look at the [the ESLint documentation](http://eslint.org/docs/user-guide/configuring),


the the ESLint 😛

Arcanemagus · 2017-10-30T16:31:42Z

package.json

+        },
+        "disableFSCache": {
+          "title": "Disable FileSystem Cache",
+          "description": "Paths of node_modules, .eslintignore and others are cached",


I know you didn't change the wording here, but since we are touching it we might as well update a few things. Can you change this to "... others are normally cached"? The way it's currently worded the description seems to indicate enabling this option enables caching, when the title (and behavior) do the exact opposite.

I was planning to take another pass at the descriptions after this PR, I think there are a few things that could probably be clarified. I'll make this change now though since you've pointed it out and we may as well.

Arcanemagus · 2017-10-30T16:32:55Z

package.json

-    "rulesToSilenceWhileTyping": {
-      "title": "Silence specific rules while typing",
-      "description": "Useful when Atom fixes errors on save like `no-trailing-spaces` or `eol-last`.",
+    "eslintRulesDirs": {


Is there any reason this isn't in the advanced section?

I suppose it could be. One of the big things that made ESLint different from JSHint was that you could write your own rules and they could be included at execution time. Once-upon-a-time the only way was by giving a directory of rules files. Now of course there are plugins, but I don't have a great sense for how many users out there still use custom rules in a rules directory.

Interesting, I guess I just assumed that anyone writing their custom rules would format them as a plugin so they would work like all other custom rules.

Arcanemagus · 2017-10-30T16:33:57Z

package.json

+          "default": false,
+          "order": 2
+        },
+        "showRuleIdInMessage": {


🗒 One could make the argument this should go in the "general uncategorized" area, but I think it's fine leaving this down here.

I don't really understand why someone would want to turn that off. Do you know off-hand the reason for the option to exist at all?

It would make the output cleaner and easier to read at the cost of being much harder to look up rules in the docs or write inline disable rules. I would never turn this off, but I can see the motivation. I think it's reasonable to argue for removing it entirely.

Looks like it was added in 72cc3cb, I was hoping for a bit more context as to why, but no luck.

If I remember correctly he wanted to remove it completely when doing the rewrite, but I made him keep it around (this was before the rule ID even linked to the relevant page, so it was very helpful for googleing an error). The compromise was an option that defaulted enabled.

When I said you could argue to remove it, I meant you could remove the setting. Removing the rule id for everybody would be incredibly mean.

Oh personally I would be for removing the setting, but there are people out there that don't like the "noise".

Arcanemagus · 2017-10-30T16:37:33Z

package.json

-      "description": "Have the linter ignore all fixable rules during linting when editing a document. The list is automatically updated on each lint job, and requires at least one run to be populated. Only supported when using ESLint v4+.",
-      "type": "boolean",
-      "default": false
+    "advanced": {


We can add a collapsed attribute to make this section collapsed by default.

Although it doesn't seem to be documented, it's tested in the specs.

Filed atom/atom#16030 about getting it officially documented 😉.

Arcanemagus · 2017-10-30T17:01:56Z

src/migrate-config-options.js

+   * eslintRulesDirs{Array<String>}. Remove in the next major release,
+   * in v8.5.0, or after 2018-04.
+   */
+  const oldRulesdir = atom.config.get('linter-eslint.eslintRulesDir')


Do we want to integrate this into the data structure above as a migrationFunc or something like that? If it exists it gets called instead of the default migration function.

Yeah I can play around with that.

Arcanemagus · 2017-10-30T17:03:48Z

src/migrate-config-options.js

+
+  // Copy old settings over to the new ones, then unset the old setting keys
+  directMoveMigrations.forEach((migration) => {
+    const oldSetting = atom.config.get(migration.old)


I think it might be worth it to do a single atom.config.get('linter-eslint') and then do our further processing over the return from that instead of calling atom.config.get(key) on each value, especially since we are migrating essentially every setting we have.

skylize · 2017-10-30T17:45:38Z

eslintrcPath --> Global. Typical use would be with a global eslint.
Maybe fixOnSave --> Advanced?
- I think it's not that easy for people to get this behaving right. It certainly should not be the very first setting, encouraging everybody to dive in asap.
- Or maybe a Fix On Save section with:
  - fixOnSave
  - ignoreFixableRulesWhileTyping
  - rulesToDisableWhileFixing
  - rulesToSilenceWhileTyping
disableEslintIgnore --> Disabling. Seems fairly innocuous and self-explanatory.
showRuleIdInMessage --> general. Nothing even slightly advanced here.

Arcanemagus · 2017-10-30T17:53:13Z

eslintrcPath --> Global. Typical use would be with a global eslint.

👍

Or maybe a Fix On Save section

I rather like the idea of grouping everything related to fixing on save together as well.

disableEslintIgnore --> Disabling. Seems fairly innocuous and self-explanatory.

That section is about turning off messages, not turning them on 😉. In any case I'd say this is a rather advanced option that should only rarely be enabled (it was added for ESLint itself development...).

IanVS · 2017-10-30T18:12:13Z

Thanks for the feedback guys! I'll take a crack at some cleanup tonight.

disableEslintIgnore --> Disabling. Seems fairly innocuous and self-explanatory.

That section is about turning off messages, not turning them on 😉. In any case I'd say this is a rather advanced option that should only rarely be enabled (it was added for ESLint itself development...).

Yeah, the section name probably isn't great, but it's more about disabling ESLint warnings for various situations. Perhaps it would be better named Disabling Linting or Disabling Warnings or something like that.

It’s out-of-date, and not actually even running right now.

By default, Atom will sort settings alphabetically, which can result in a really strange, non-logical order. This commit attempts to put more important or commonly used settings up top, while also grouping similar settings together. So, the scopes are together with the lint html setting, the use global and global paths are together, the fixing options are grouped, etc. I’m sure this isn’t perfect, but it’s a start.

This creates an options group for settings that disable linting in one way or another. Because moving options into groups will require a lot of migrations, this also creates a new file/function to handle those migrations, thus keeping them out of `main.js`. There may be a more elegant way to manage the migrations, but this gets us started.

For standard migrations, all we really need to do is check if the old setting is defined, then move it to the new setting, then unset the old setting. We can just make a simple loop for this, keeping the old and new setting strings as data, apart from the logic that moves them.

These are settings that are not commonly needed, so we may as well try to avoid confusing new users by indicating that they are “advanced”

IanVS · 2018-01-28T03:11:02Z

showRuleIdInMessage --> general. Nothing even slightly advanced here.

I guess. I just wanted to hide it because I think it's a dumb option. :-p

And also collapse advanced section.

skylize · 2018-01-28T04:18:21Z

package.json

+      "properties": {
+        "useGlobalEslint": {
+          "title": "Use global ESLint installation",
+          "description": "Make sure you have it in your $PATH",


Could we add something here to emphasize that local installation is the suggested usage?

As I mentioned to @Arcanemagus in one of the previous comments, I'd like to save wording tweaks for another PR. There is a lot that I would like to do to help explain and clarify our settings, but this PR is really just about reorganizing them.

Fair enough. 👍

IanVS · 2018-01-28T04:44:46Z

src/migrate-config-options.js

+ */
+const migrations = [
+  {
+    type: 'direct-move',


Now that I think about it, this type is pointless. Can just check for a moves or migrationFunc key directly. Will change.

And migrate is maybe a better name than migrationFunc. SMH.

I like the clarity and declarative nature of the direct move array. But yeah, I think I agree there is unnecessary convolution in directing it there.

Definitely agree with the rename.

(we don’t have babel configured for it)

Arcanemagus

Bug in the migration code is the only needed change.

Arcanemagus · 2018-01-29T22:48:37Z

src/migrate-config-options.js

+      migration.moves.forEach((move) => {
+        const oldSetting = linterEslintConfig[move.old]
+        if (oldSetting !== undefined) {
+          atom.config.set(move.new, oldSetting)


Missing the linter-eslint package name on the settings key.

Arcanemagus · 2018-01-29T22:48:54Z

src/migrate-config-options.js

+        const oldSetting = linterEslintConfig[move.old]
+        if (oldSetting !== undefined) {
+          atom.config.set(move.new, oldSetting)
+          atom.config.unset(move.old)


Missing the linter-eslint package name on the settings key.

Arcanemagus · 2018-01-29T22:51:52Z

src/migrate-config-options.js

+ * version.
+ */
+function migrateConfigOptions() {
+  const linterEslintConfig = atom.config.get('linter-eslint')


🤔 What about only grabbing this in the forEach if it's not already set?

It's not quite as clean, but it does save grabbing the entire settings if there are no migrations to perform.

Based on the way this is laid out, I don't see any situation where your suggestion would somehow prevent grabbing the config. The if check is just so he can handle eslintRulesDirs differently than the straight renames.

I'm thinking about later, when we completely deprecate and remove the current migrations. Unless we are going to keep them around forever.

Well as long as we're messing around with it, I think this could be much more clear by increasing the symmetry between different types of migrations, as in they should all be function calls, which also removes the need to check if moves exists. So combining that with your "maybe don't grab config", that would probably look something like this.

const migrations = [ { added: 'January, 2018', description: 'Organized config settings into sections', migrate: (config) => { const moves = [ /* ... list of moves ... */ ] moves.forEach((move) => { const oldSetting = config[move.old] if (oldSetting !== undefined) { atom.config.set(move.new, oldSetting) atom.config.unset(oldSetting) } }) } }, { added: 'September, 2017' // ... } ] // If nothing to migrate don't grab config const linterEslintConfig = migrations.length ? atom.config.get('linter-eslint') : null // If nothing to migrate, no problem that config is // null because this is a no-op. migrations.forEach(migration => migration.migrate(config))

I'd move that inner (move) => function out to it's own definition since it will likely be used elsewhere, but that does look cleaner.

Arcanemagus · 2018-01-29T22:53:09Z

package.json

+          },
+          "order": 4
+        },
+        "advancedLocalNodeModules": {


🤔 Since we are already migrating this to a new key, what about renaming it to just localNodeModules?

Since it has a prefix of `advanced` now anyways.

I believe this is what was intended originally.

skylize · 2018-02-12T02:37:43Z

spec/migrate-config-options-spec.js

+import migrateConfigOptions from '../src/migrate-config-options'
+import makeSpy from './make-spy'
+
+describe('migreateConfigOptions()', () => {


I do think it's funny that you also had a typo, in pointing out my typo:

typo her

IanVS · 2018-02-12T02:37:59Z

@Arcanemagus I fixed the bug in the migration code and added some specs. I opted not to refactor as suggested in #1042 (comment), mostly because it ended up making things more complicated. Specifically, I foresee that most migrations we do will be simple moves, and having a moves array is pretty dead simple. Also, testing became a bit more troublesome when using a migrate function even for simple moves, because then I would need to also export the helper function and honestly it just didn't all seem worth it. If you think it's a blocker, then I can re-address, but I'm kinda at the point where I'd just like to be done with this. :)

skylize · 2018-02-12T02:45:11Z

It seems to get the job done. I'm not gonna stand in the way. We can always iterate. (If we manage to get fun-functional merged in, it all gets changed anyway to fit better with the newer config subscription setup.)

Arcanemagus · 2019-01-28T04:32:32Z

🎉 This PR is included in version 8.4.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

IanVS requested a review from Arcanemagus October 30, 2017 04:40

IanVS force-pushed the organize-config-options branch from 4709c8b to 4ff49c3 Compare October 30, 2017 12:30

Arcanemagus added the enhancement label Oct 30, 2017

Arcanemagus suggested changes Oct 30, 2017

View reviewed changes

IanVS added 8 commits January 27, 2018 21:44

Remove unused docs images

9826f5c

Remove mention of blog post

57ceb39

It’s out-of-date, and not actually even running right now.

Group “advanced” settings

7e8b0f5

These are settings that are not commonly needed, so we may as well try to avoid confusing new users by indicating that they are “advanced”

Group global eslint options together

27631af

Rearrange remaining options

3f12099

IanVS force-pushed the organize-config-options branch from 4ff49c3 to 3f12099 Compare January 28, 2018 02:50

IanVS added 2 commits January 27, 2018 21:58

Wording tweaks

2088026

Move eslintrcPath setting to global section

da1ac1a

IanVS force-pushed the organize-config-options branch from 9f07b6a to 3f254d9 Compare January 28, 2018 04:06

IanVS added 3 commits January 27, 2018 23:24

Move eslintRulesDirs to advanced section

03a7979

And also collapse advanced section.

Create an autofix section

73ea71b

Do a single atom.config.get

398ecb9

IanVS force-pushed the organize-config-options branch from 3f254d9 to 1d29119 Compare January 28, 2018 04:24

skylize suggested changes Jan 28, 2018

View reviewed changes

skylize approved these changes Jan 28, 2018

View reviewed changes

IanVS commented Jan 28, 2018

View reviewed changes

Refine migration approach slightly

fdb5de9

Avoid object rest spread

554ca65

(we don’t have babel configured for it)

IanVS force-pushed the organize-config-options branch from 1d29119 to 554ca65 Compare January 29, 2018 13:36

Arcanemagus suggested changes Jan 29, 2018

View reviewed changes

IanVS added 2 commits February 11, 2018 21:33

Rename advancedLocalNodeModules to localNodeModules

49b695c

Since it has a prefix of `advanced` now anyways.

Make “called” method of spy return a boolean

4aa80d7

I believe this is what was intended originally.

IanVS force-pushed the organize-config-options branch from cd10d4f to a52f23e Compare February 12, 2018 02:33

skylize reviewed Feb 12, 2018

View reviewed changes

Fix migrate-config-options and add specs

ccb881e

IanVS force-pushed the organize-config-options branch from a52f23e to ccb881e Compare February 12, 2018 02:38

Arcanemagus approved these changes Feb 13, 2018

View reviewed changes

IanVS merged commit 0eb16c4 into master Feb 13, 2018

IanVS deleted the organize-config-options branch February 13, 2018 13:27

Arcanemagus added the released label Jan 28, 2019

Organize config options #1042

Organize config options #1042

Conversation

IanVS commented Oct 30, 2017 • edited Loading

IanVS commented Oct 30, 2017

skylize commented Oct 30, 2017

Arcanemagus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arcanemagus Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skylize commented Oct 30, 2017

Arcanemagus commented Oct 30, 2017

IanVS commented Oct 30, 2017

IanVS commented Jan 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skylize Jan 28, 2018 • edited Loading

Choose a reason for hiding this comment

Arcanemagus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skylize Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skylize Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanVS commented Feb 12, 2018

skylize commented Feb 12, 2018

Arcanemagus commented Jan 28, 2019

IanVS commented Oct 30, 2017 •

edited

Loading

Arcanemagus Oct 30, 2017 •

edited

Loading

skylize Jan 28, 2018 •

edited

Loading

skylize Jan 30, 2018 •

edited

Loading

skylize Feb 12, 2018 •

edited

Loading