Ternary line breaking

[prettydiff]

var a = (tern)
    ? b
    : c;

This code produces the warning: Bad line breaking before '?'.

The jshint flow control is:

First catch the "?" token and then pass it to the expression function

  infix("?", function(left, that) {
    increaseComplexityCount();
    that.left = left;
    that.right = expression(10);
    advance(":");
    that["else"] = expression(10);
    return that;
  }, 30);

Function expression contains this logic near it's start

    var isDangerous =
      state.option.asi &&
      state.tokens.prev.line !== startLine(state.tokens.curr) &&
      _.contains(["]", ")"], state.tokens.prev.id) &&
      _.contains(["[", "("], state.tokens.curr.id);

    if (isDangerous)
      warning("W014", state.tokens.curr, state.tokens.curr.id);

It appears the JSHint code is incorrectly catching the line break between the ")" and the "?" in its ASI logic. ASI must not apply in this case. Breaking a ternary onto multiple lines makes the code easier to read. Example:

var a = (tern1 > 0)
        ? b
        : (tern2 > x)
            ? c
            : d,
    e = 1;

[erezfz]

why are you still using JSHint, and not only JSLint which is much better?

[prettydiff]

@Kaizer200 JSHint is far more frequently used tool. My goal is to attain conformance to the default settings of both as closely as I can. This way I can accurately represent any user wishes to conform to either tool with the fewest possible adjustments.

[nicolo-ribaudo]

I think this is wanted, as there is this test:

code = [ 'var a = b ', '? c : d;' ];
TestRun(test)
  .addError(2, "Bad line breaking before '?'.")
  .test(code, { es3: true });

(tests/unit/options.js, line 1583)

[caitp]

The fact that there's a test doesn't mean it's wanted. This is more jscs's territory

[nicolo-ribaudo]

@caitp Are you fixing this?

[lukeapage]

This isn't disabled by laxbreak? I know its deprecated, but it won't be removed till v3

[caitp]

There's no real roadmap for v3, it's safe to get rid of this --- it won't break anyone's builds

[nicolo-ribaudo]

@caitp For the same token, JSHint should also allow line breaks before &&, ||, ... by default.

[lukeapage]

There is a roadmap, me and @jugglinmike have discussing it.its everything on the v3 milestone.

It will not break peoples builds, but it will stop warnings people rely on, when there is no warning and very little documentation that the option has been removed (and no major version bump to make people look at the changelist and notice they should use jscs)

[caitp]

Nobody is realistically relying on this, and a milestone isn't a roadmap. The v3 milestone has been around for ages, there's no deadline and no pressure. There is a change log for all releases, not just major ones, and other bogus warnings have been removed already. There is no compelling argument to keep this around indefinitely, it is jscs territory

[lukeapage]

The v3 milestone has been around for ages

We recently cleared it up and made it into a shorter, achievable list.

There are a couple of PR's waiting for it.

There is a change log for all releases, not just major ones, and other bogus warnings have been removed already. There is no compelling argument to keep this around indefinitely, it is jscs territory

I agree, its just when. I just disagree with both classing do not warn about deprecated rules as being a breaking change, so not merging till v3 and at the same time, removing deprecated rules. If jshint wants to be ultra careful, be ultra careful. If it wants to remove rules, bump a major version or warn people.

Its pretty easy to miss that the options have become deprecated if you set jshint up some time ago - do we want the experience that people get is for them to raise a bug and then get told its deprecated? or make it obvious?

[lukeapage]

Nobody is realistically relying on this

I used to rely on jshint for some kind of style checking until I found out about jscs and switched to it. I bet alot of people still are.

[nicolo-ribaudo]

Nobody is realistically relying on this

Searching on GitHub... https://github.com/search?l=json&q=%22laxbreak%22%3A+false&ref=searchresults&type=Code&utf8=%E2%9C%93

[lukeapage]

@prettydiff
this passes jshint

/* jshint laxbreak: true */
var a = (tern)
    ? b
    : c;

So, just turn that option on to not get warnings.

laxbreak will be removed in the near future. this is the issue raised for removing it - #1867

[lukeapage]

any objection to closing this as a duplicate of #1867 ?

[caitp]

My vote is to make it a fresh bug and fix this properly. No major version bump it's required here

[prettydiff]

@lukeapage If laxbreak is removed in v3.0 will this code sample generate warnings in v3.0 and what is the approximate timeline on a v3.0 release?

[lukeapage]

If laxbreak is removed in v3.0 will this code sample generate warnings in v3.0

The plan is to remove the laxbreak option and no longer warn in cases of laxbreak.
The use of the option may generate a warning that the option is deprecated and can be removed.

what is the approximate timeline on a v3.0 release

I'm not sure, there is a que of pr's to review/finalise and get in which fix alot of bugs. I doubt it will happen in the next month.

Cait is a more long term contributor and other contributors/owners might decide to agree with her and remove the warnings in the next release, at which point you would stop seeing warnings and then in v3 you would start getting warned that the laxbreak option is deprecated (should you decide to start using that now).

[prettydiff]

@lukeapage With that you may now close this bug (from my perspective).