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

operator-linebreak: allow "ignore" override [$15] #4294

Closed
dmsnell opened this Issue Oct 29, 2015 · 19 comments

Comments

Projects
None yet
6 participants
@dmsnell
Copy link

commented Oct 29, 2015

It would be great if we could tell this rule to ignore certain operators. For example, in our code we use both of these styles:

render() {
    return (
        { this.shouldRender() ?
            <MyComponent />
        : null }
    );
}
const editorComponents = this.isAdvancedView()
    ? <MyAdvancedComponent />
    : <MySimpleComponent />;

Both cases feel legitimate, but there's no way to make operator-linebreak happy, because one has the line break before and one after in the same module.

Occasionally we do something really wacky.

render() {
    return (
        { this.isWacky ?
            <Some>
                <Component>
                    <Tree />
                </Component>
            </Some>
        :
            <Another>
                <Component>
                    <Tree />
                </Component>
            </Another>
        }
    );
}

So this case isn't as important because you can make lots of arguments against having such a structure, but it demonstrates how here the : looks best with a line break both before and after it.

We love the rule for other operators, but could we potentially add in an "ignore" flag to just skip these? Thanks!

Did you help close this issue? Go claim the $15 bounty on Bountysource.

@eslintbot

This comment has been minimized.

Copy link

commented Oct 29, 2015

Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Oct 29, 2015

dmsnell added a commit to dmsnell/eslint that referenced this issue Oct 29, 2015

Add "ignore" rule to skip processing for a given operator
Proposed patch for [eslint#4249](eslint#4294) to add an "ignore" value for the overrides to allow `eslint` to skip the rule for a particular operator.
@nzakas

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

Are you proposing adding an option to ignore just ?:? Or to ignore any random operator? Or something else?

@dmsnell

This comment has been minimized.

Copy link
Author

commented Oct 29, 2015

@nzakas thanks for the quick response!

I'm proposing an option to allow "ignore" in the override section, workable for any operator. In our case, we would probably just override ? and :, but maybe for some reason someone would want to use + in a strange way and ignore that one too.

Honestly I just thought it would be simpler and more standard to allow the per-operator override than to make a special case for those two.

"operator-linebreak": [ 1, "after", { "overrides": {
    "?": "ignore",
    ":": "ignore"
} } ],
@nzakas

This comment has been minimized.

Copy link
Member

commented Oct 30, 2015

I'm on the fence here. We've historically shied away from nondeterministic options on rules as it can be confusing to end users about what is expected. On the other hand, this is similar to putting in /*eslint-disable*/ comments around these operators, so maybe it's not that big of a deal.

@eslint/eslint-team thoughts?

@dmsnell

This comment has been minimized.

Copy link
Author

commented Oct 30, 2015

@nzakas the issue with the inline comment is that it pollutes the code, especially if the problem is a project-wide style. We'd have to put hundreds of those comments in and they would end up uglier than doing the tertiaries the way we don't want to.

What is the non-deterministic part of ignoring an operator?

@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Oct 30, 2015

I'm not sure I can get behind this proposal. It feels like it goes directly against the original purpose of the rule. Why have the rule on if you know you are not following the pattern that the rule suggests? It feels like the right solution in this case is just to turn off this rule.

@dmsnell

This comment has been minimized.

Copy link
Author

commented Oct 30, 2015

@ilyavolodin am I totally wrong here? does this not also check things like + and - and || and &&? We would love to make sure those are still following the appropriate convention, but simply want to ignore tertiaries from the rule. Sorry if I don't understand the rule or its purpose.

@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Oct 31, 2015

@dmsnell The rule is made to enforce a given style. Either break follows an operator, or operator follows a break. As far as I understand, what you are looking for is more or less (sorry, I might be oversimplifying here) not to follow the style, but still have this rule on. In some cases you want to have operator following line break, and in some - preceding. I understand that it's also a style on it's own, but it's not the style that this rule was created for. At least that's the way I'm thinking about it.

@dmsnell

This comment has been minimized.

Copy link
Author

commented Oct 31, 2015

thanks for the clarification @ilyavolodin - is there any use in keeping this rule active if we were to ignore ternaries? In other words, does this rule already ignore every other operator in which case something like "ternary-linebreak" might be a better description?

@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Oct 31, 2015

@dmsnell No this rule doesn't ignore other operators. Here's an example that will be flagged by default configuration:

var a = 2
    + 2;

ternary operators are the only one that have a default exception. As in, all of the operators (-,+,=,||,&&, etc.) should be preceding linebreak, but ? and : should be following linebreak with the default configuration of the rule. I'm honestly not sure why this was setup this way by default.

@dmsnell

This comment has been minimized.

Copy link
Author

commented Nov 1, 2015

Thanks @ilyavolodin - it's an interesting choice here. Maybe we'd be better simply to not run operator-linebreak as I don't think we have many issues with the other operators and could probably do without it. All in all, I think the ignore rule might help us the most, but I understand that doesn't really fall into the goals for this rule.

@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Nov 1, 2015

@dmsnell Thanks for understanding. We have to cater our rules to large audience and balance that with maintainability. My suggestion is that if you only care about operators other then ternary, you might want to copy existing operator-linebreak, modify it, and create a plugin out of it. That's probably the best way for your style.

@nzakas

This comment has been minimized.

Copy link
Member

commented Nov 2, 2015

@ilyavolodin not sure I agree with your read here. When we have a rule that applies to a bunch of things (as in this case), and we already have a way to modify which things it applies to, it seems like allowing one of those options to be "don't apply to this" could be useful. We have other rules that allow omissions (like in no-shadow), so maybe this is OK?

@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Nov 2, 2015

@nzakas We always sort of tried to stay away from tri-state switches. I'm not generally opposed to having a way to ignore certain operators, what I'm saying is that it wasn't what the rule was designed to do. If you are fine changing the target of the rule, I'm not going to complain about it. I just think before, after and ignore are not self-explanatory and might confuse users.

@nzakas

This comment has been minimized.

Copy link
Member

commented Nov 2, 2015

I'm not sure I understand what you mean by this wasn't the rule was designed to do. The rule was designed to provide spacing around operators while allowing overrides - this seems like it's just another override.

I don't think adding such an option changes what the rule is meant to do, its just mimicking what can already be done using comments.

Am I missing something?

@gyandeeps

This comment has been minimized.

Copy link
Member

commented Dec 21, 2015

I think we have had a good discussion here, can we regroup to flush through this issue please?
CC: @nzakas @ilyavolodin ... we are almost thr... 😄

@nzakas

This comment has been minimized.

Copy link
Member

commented Dec 22, 2015

I think we can do this if someone wants to submit a PR

@nzakas nzakas changed the title operator-linebreak: allow "ignore" override operator-linebreak: allow "ignore" override [$15] Dec 27, 2015

@nzakas nzakas added the bounty label Dec 27, 2015

@rpatil26

This comment has been minimized.

Copy link
Contributor

commented Jan 1, 2016

I have decided to give it a try. Give me few hours.

@rpatil26

This comment has been minimized.

Copy link
Contributor

commented Jan 1, 2016

Almost done. I am not going to support it in global option as it doesn't makes sense. It ("ignore") will only be supported using overrides. Hope that's fine.

rpatil26 added a commit to rpatil26/eslint that referenced this issue Jan 1, 2016

Update: Add "ignore" override for operator-linebreak (fixes eslint#4294)
To support "ignore" in overrides for specific operator/s.

@nzakas nzakas closed this in 9c9beb5 Jan 5, 2016

nzakas added a commit that referenced this issue Jan 5, 2016

Merge pull request #4841 from rpatil26/issue4294
Update: Add "ignore" override for operator-linebreak (fixes #4294)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.