"Repository settings" as a way to share configuration between rules without creating rule-to-rule dependencies?

[platinumazure]

We continue to run into questions about how best to auto-fix rules that appear to have a dependency on other rules.

For example, lines-around-comment and newline-before-return may introduce blank lines as part of its auto-fix, but it has no way of knowing the correct line break sequence to use (which may be configured in linebreak-style).

Now that we have multi-pass autofixing available, a common response is that we can let multi-pass autofix take care of it. For example, lines-around-comment can arbitrarily choose \n (without loss of generality) and that will be applied in the first pass, and then the second pass of autofix can fix the line break issues. However, the problem with this reasoning is that not everyone will use those rules that apply second-pass fixes.

Noting that no rule can be deemed "too important to turn off" (meaning we really shouldn't be recommending the use of other rules that users might not want to use), I think we need to consider a different approach.

Below is a proposal for how we can better handle this. In terms of scope, I think we can start with something simple, like line breaks; later on, we can augment this proposal to include other repository style elements as needed.

In configuration:

{
    "rules": {
        "linebreak-style": "error",
        "lines-around-comment": "error",
        "newline-before-return": "error"
    },
    "repositorySettings": {
        "lineBreak": "unix"
    }
}

Changes to how the affected rules would work:

• Repository settings becomes available in RuleContext object
• linebreak-style rule uses repository setting lineBreak property if specified
  • Backward compatibility: can still accept inline config for backward compatibility until next major version.
  • If lineBreak is specified, then the goal of this rule is solely to enforce the style specified in repositorySettings.
• lines-around-comment uses repository setting lineBreak property if specified when autofixing
  • Backward compatibility: Could fallback to arbitrary line termination sequence such as \n if needed
• newline-before-return uses repository setting lineBreak property if specified when autofixing
  • Backward compatibility: Could fallback to arbitrary line termination sequence such as \n if needed
• I haven't accounted for other rules that may need this information, but my assumption is they can migrate toward using repositorySettings in minor version bumps.

Other thoughts:

• Repository settings probably deserve their own JSON Schema
• Rule metadata could be used to specify dependencies on repository settings, and ESLint could error out if repository setting not specified, similar to errors we emit for mis-configurations
  • Needless to say, for existing rules to actually use these dependencies and error out, that would have to wait for a major version bump

(N.B. I had created an issue with a similar title in the past, but the discussion there was focused around plugin processors and was correctly closed. I hope the team can agree that this issue is sufficiently different from the old issue and that the motivation for discussion is highly relevant now as we try to avoid over-reliance on multi-pass autofix.)

[michaelficarra]

Yes yes yes yes yes. I would go a (large) step further and move all rule configuration into one large shared configuration space. Rules themselves shouldn't have options. Projects have preferences which are unrelated to rules. Multiple rules may need to reference the same preference, especially when it is a style preference. We have seen the problems that arise when one rule cannot reference another rule's configuration. Hopefully this proposal pushes the project in that direction. 👍 👍 👍

[ilyavolodin]

While I'm not in love with current way we are doing autofixing, can you explain what you think this proposal be able to provide other than what we already have? A change as drastic as this should have very clear benefits, but I'm not sure I can see them. I also see a lot of complication in the way configs cascade (as it relates to autofixing and project configuration), and inline configurations.

[platinumazure]

The "clear benefit" is that linebreak-style, lines-around-comment, and newline-before-return can look in one shared place for knowing what the correct linebreak should be.

I'm not sure I agree about complications in configuration cascade:

• repositorySettings can be merged the same way as anything else-- for each key/value pair, the config file in the descendant directory prevails (just like envs, rules, everything else).
• I don't believe we should necessarily allow inline configuration for repositorySettings, but I could see something like /* eslint-repository-setting lineBreak:unix */ for those cases where it might be absolutely necessary. I would be in favor of adding it only on demand.

[ilyavolodin]

@platinumazure Let me rephrase it. What does this change allows us to do that we can't do already? Currently we can fix all of the rules you listed above without knowing what the correct linebreak style is supposed to be. Are there any other benefits?

[platinumazure]

@ilyavolodin No we can't, not if the user decides not to use linebreak-style. It is irresponsible of us to elevate the responsibility of some rules in order to allow other rules to work better or correctly.

[ilyavolodin]

Ah, OK, now I think I got where you are going with this. So you expect every used to configure every single possible option of repositorySettings? What if I really don't care about what type of linebreaks my code uses and don't want to waist my time creating a style guide?

[mysticatea]

I may not understand this issue, but 👎 from me.

There are 2 reasons.

---

I'm not sure there are what difference between:

{
    "rules": {
        "linebreak-style": ["error", "unix"],
        // some rules which inserts linebreak.
    }
}

And

{
    "rules": {
        "linebreak-style": "error",
        // some rules which inserts linebreak.
    },
    "repositorySettings": {
        "lineBreak": "unix"
    }
}

I think if people turn linebreak-style rule on, it means "I want to require consistent line break style.". Then ESLint would work as expected.
On the other hand, repositorySettings.lineBreak is not so. If people forget linebreak-style rule, they cannot get consistent line breaks. E.g. someone's editor might use different line break style.

In short, I think linebreak-style itself is a repository setting.
I cannot see any reason to separate the setting of line break style from linebreak-style.

---

I think a rule should focus on the one responsibility of the rule, then it should leave other matters to other rules. I don't think linebreak-style is special one. Let's think more cases:

• object-curly-newline and linebreak-style

  Should object-curly-newline rule consider linebreak style when it inserts a line break?

  let obj = {a: 1};
  
  let obj = {\n
  a: 1\n
  };
  let obj = {\r\n
  a: 1\r\n
  };

• object-curly-newline and indent

  Should object-curly-newline rule consider indent when it inserts a line break?

  let obj = {a: 1};
  
  let obj = {
  a: 1
  };
  let obj = {
      a: 1
  };

• object-curly-newline and no-trailing-spaces

  Should object-curly-newline rule consider trailing spacing when it inserts a line break?

  let obj = { a: 1 };
  
  let obj = {
      a: 1•
  };
  let obj = {
      a: 1
  };

• object-curly-newline and object-curly-spacing, no-multi-spaces

  Should object-curly-newline rule consider spacing when it removes a line break?

  let obj = {
      a: 1
  };
  
  let obj = {    a: 1};
  let obj = {a: 1};
  let obj = { a: 1 };

• object-property-newline and comma-spacing, no-multi-spaces

  Should object-property-newline rule consider spacing when it removes a line break?

  let obj = {a,
      b};
  
  let obj = {a,b};
  let obj = {a, b};
  let obj = {a,    b};

I'm afraid that the responsibility of a rule would spread unlimitedly.
And I don't think linebreak-style is a special one.

[platinumazure]

Well, let me just make sure I lay out the motivation behind this proposal, in case it's not clear:

1. I acknowledge that some rules, in order to work correctly, depend on settings which are currently stored in other rules.
2. We have a design rule which says rules cannot depend on each other (directly).

The basic thrust of my proposal is to extract some of those settings into a shared repository, so that rules depend on that repository rather than each other.

Maybe @michaelficarra is right and I'm not thinking big enough. 😄 Perhaps he could share his thoughts? I also had an idea about having ESLint support integration with other things, like .editorconfig; maybe that would be less confusing for some users.

[ilyavolodin]

@platinumazure regarding your motivations. Can you give an example of rule that depends on the configuration of another rule? If you forget about fix mode for a second, is there really any case where rules depend on each other? If we are talking purely about fix mode, then multi-pass seems to address majority of the problems. Granted, it's much slower and less efficient, but it does work. What's more, is that if you create repositorySettings, you are coupling rules together. It's not a direct coupling, but now multiple things depend on an existence of a single property. In the current setup, every rule is completely decoupled and works independently without sharing anything with other rules.
Again, to reiterate, I'm not in love with our multi-pass approach, mainly due to a huge overhead in terms of performance and IO, but I'm not sure that this proposal will allow us to fix those concerns, and it's a huge change, that requires modification of more or less every aspect of ESLint, and I'm not seeing enough benefits to justify that right now.

[mysticatea]

I acknowledge that some rules, in order to work correctly, depend on settings which are currently stored in other rules.

This recognition is different from me. Why do you think so?

[platinumazure]

@ilyavolodin The issue I'm trying to solve is mostly caused by auto-fix. Auto-fix can introduce violations of other rules. Most of the time, we do a good job of figuring out direct rule conflicts (e.g., we made a decision that the space between if and a parenthesis is in keyword-spacing, not in a different rule). However, when we designed auto-fix, we didn't rigorously design a way that rules could auto-fix themselves without causing violations of other rules (and there was no way to do this out of the box due to the fact that rules cannot reference each other). I agree with the norm that rules should not reference each other, but the reality is our auto-fix approach is flawed due to rule independence and we need to come up with a better approach. If a better approach (than my suggested repositorySettings) were to arise, I would get behind that.

As I mentioned before, multi-pass cannot be the solution for this issue, simply because a user might not know that a second rule needs to be turned on in order for a given rule to auto-fix "correctly" (after multiple passes).

@mysticatea To be clear, I'm talking about auto-fixing without introducing violations. In terms of linting only, the rules generally do not clash with each other, and when they do, we do a pretty good job of designing around that. Right now, auto-fix causes clashing with other rules, because the wrong fix is done (because we don't have enough information available in many cases, because that information is assigned to other rules).

[ilyavolodin]

@platinumazure What would happen in your proposal if I didn't add linebreak property to repositorySettings? Either I forgot to do it, or I just don't care, because I'm using .editorconfig. But then I enabled linebreak-style rule? What's the expected behavior in that case, and how should autofix behave for lines-around-comments for example?

[mysticatea]

@platinumazure So... I mentioned some questions in my comment above. Your answers seem all yes. If so, I'm afraid that the responsibility of a rule would spread unlimitedly. object-curly-newline needs to consider the same things as linebreak-style, indent, object-curly-spacing, no-trailing-space, no-multi-spaces, and unknown plugin rules (cannot).

[alberto]

My main concern is with introducing the wrong linebreak in a fix, because they are invisible chars. If I were developing alone on windows (using \r\n) I might think I don't need to enable linebreak-style, since it's just me working on it, and wouldn't notice eslint added \ns.

[mysticatea]

If you don't turn linebreak-style on, it means "I don't require consistent line break style." for ESLint, I think.

[mysticatea]

I'm not sure, why do you think that separating the setting linebreak style from linebreak-style solves your concern?

{
    "rules": {
        "a-newline-rule": "error"
    },
    "repositorySettings": {
        "linebreak-style": "\n"
    }
}

{
    "rules": {
        "a-newline-rule": "error",
        "linebreak-style": ["error", "\n"]
    }
}

I don't think major difference exists between that 2 configs.
In the former case, ESLint does not check the consistency of line breaks, though....

[SpadeAceman]

👎 As I noted in this comment, not only would people need to understand how to use the ~200 rules currently available, but they'd also need to be aware of the (often unexpected) side-effects of using certain repository settings along with those rules.

The repository settings would effectively become globals, with all of the problems that implies. People will inevitably want rules to respect the global repository settings in some circumstances while ignoring them in others.

It's already a struggle to understand how to properly use all of the existing rules, but at least they're self-contained. By creating hidden external dependencies upon global repository settings, rules would become far more complex and difficult to understand or troubleshoot.

A better approach would be to allow the user to create a "variables" repository, letting them name their variables however they want, and then giving them a way to pass those variables to rules as if they were regular values. (For instance, the user could define a "my-line-break" variable with value "unix", then pass that variable to linebreak-style, lines-around-comment, etc.) This would avoid the hidden dependency issues inherent in the original proposal here, by requiring all variables to be explicitly passed to the rules using them.

[pedrottimark]

Two questions to make explicit something that I missed until just now:

• Are “repository settings” a specific type of settings as described in http://eslint.org/docs/user-guide/configuring#adding-shared-settings?
• Are the generic settings intended for plugins only and not to be used by core rules?

[platinumazure]

A better approach would be to allow the user to create a "variables" repository, letting them name their variables however they want, and then giving them a way to pass those variables to rules as if they were regular values. (For instance, the user could define a "my-line-break" variable with value "unix", then pass that variable to linebreak-style, lines-around-comment, etc.) This would avoid the hidden dependency issues inherent in the original proposal here, by requiring all variables to be explicitly passed to the rules using them.

The drawback to this is the need to repeat some of the values (e.g., your line break is referenced in a bunch of rules, so extra typing of either "\n"/"\r"/"\r\n"`, or a variable name referencing the same). But I can see the positives as well.

Two questions to make explicit something that I missed until just now:

• Are “repository settings” a specific type of settings as described in http://eslint.org/docs/user-guide/configuring#adding-shared-settings?
• Are the generic settings intended for plugins only and not to be used by core rules?

I had conceived of repositorySettings as being separate from those "settings".

And my assumption is that core rules do not use those "settings" at all, possibly for similar reasons as those cited by the folks who are 👎 on my proposal.

[alberto]

@mysticatea I'm not sure if the solution is to separate it, but allowing access to the defined linebreak-style would allow a-newline-rule to use it

[platinumazure]

Folks,

Bear in mind that the main thrust of this proposal was as a reaction to the architectural direction of "rules cannot know about each other". If we believe it is simpler to simply allow rules to know about each other, then I agree my proposal is overkill, but it is worth noting that there is a very good reason we keep the rules separate and independent.

So maybe this question should be answered before we do anything else: Is the directive "rules cannot know about each other" something we should consider an absolute, inviolable directive? Or do we have wiggle room here?

[ilyavolodin]

@platinumazure I, personally, would be very much against anything that breaks "rules cannot know about each other" change.

P.S. Keep in mind that all the negative feedback is not directed against you at all, but it seems that this approach is creating more problems that it's solving, at least in the form that it is right now.

[mysticatea]

I apologize if I'm too offensive.
I'm not familiar with ways to say opposite opinions in English.

@alberto Hmm but why do we need that a-newline-rule can access line break style? Sure, people may forget rules.linebreak-style, but people may forget repositorySettings.linebreak-style as well. It seems to improve nothing to me.

@platinumazure

it is worth noting that there is a very good reason we keep the rules separate and independent.

Maybe, this is the main difference between my thought and yours about this proposal. I think this proposal is the opposite of "rules are independent."

I think that "rules are independent" is "a rule doesn't cause to modify other rules." This is one for Single Responsibility Principle.
Currently, there is one reason that a rule causes to modify other rules. We modify a rule if the rule conflicts with another rule. This proposal would add a new reason to this. When a rule changed the behavior, even if it doesn't conflict with other rules, we may need to modify fixing logic of other rules.

This proposal increases the reason that a rule causes to modify other rules. So I think this proposal decreases the independence of rules.

[platinumazure]

@ilyavolodin:

P.S. Keep in mind that all the negative feedback is not directed against you at all, but it seems that this approach is creating more problems that it's solving, at least in the form that it is right now.

No worries, I understand. I would love to see other approaches, though.

@mysticatea:

I apologize if I'm too offensive.
I'm not familiar with ways to say opposite opinions in English.

I don't know if that was directed at me, but for what it's worth, I don't think anything you said was offensive.

I think that "rules are independent" is "a rule doesn't cause to modify other rules." This is one for Single Responsibility Principle.
Currently, there is one reason that a rule causes to modify other rules. We modify a rule if the rule conflicts with another rule. This proposal would add a new reason to this. When a rule changed the behavior, even if it doesn't conflict with other rules, we may need to modify fixing logic of other rules.

This proposal increases the reason that a rule causes to modify other rules. So I think this proposal decreases the independence of rules.

I guess from my perspective, we're already in that position even without my proposal. Rules can already cause fixing logic of other rules to be wrong.

@michaelficarra I'm really interested to know what your proposal was, which goes a step farther. Right now, I cannot tell if my proposal is too much in the wrong direction, or if it is a half-measure that might be clarified and improved upon by your proposal.

[mysticatea]

I guess from my perspective, we're already in that position even without my proposal. Rules can already cause fixing logic of other rules to be wrong.

But those would be fixed in the next pass of autofix.

[nzakas]

Unfortunately, it looks like consensus couldn't be reached on this issue and so I'm closing it. While we wish we'd be able to accommodate everyone's requests, we do need to prioritize. We've found that issues failing to reach consensus after 21 days tend never to reach consensus, and as such, we close those issues. This doesn't mean the idea isn't interesting, just that it's not something the team can commit to.