Update: Improve report location newline-per-chained-call (refs #12334)

[mdjermanovic]

Prerequisites checklist

• I have read the contributing guidelines.
• The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

What is the purpose of this pull request? (put an "X" next to an item)

[X] Other, please explain:

refs #12334

Change reported location in the newline-per-chained-call rule.

This shouldn't produce any new warnings because it doesn't change the start line.

What changes did you make? (Give an overview)

Changed location to the full location of . or [, instead of start location of the property node.

Before:

After:

Is there anything you'd like reviewers to focus on?

This looks like a big visual change. I chose this among many alternatives because it matches the place where the fixer inserts newline, and it's a common place to insert the newline manually (either on the left or on the right side). Also, the message starts with ./[

[kaicataldo]

LGTM, thanks!

[nzakas]

Since we want to draw attention to the property, not just the dot, what do people think about specifying the location to start with the dot and end at the end of the property? (To me underlining only the dot looks strange, but happy to go along with consensus.)

[kaicataldo]

I think @nzakas’s suggestion makes sense, particularly since that’s what the error message reports. 👍

[mdjermanovic]

Agreed, it would be great if we could think of something better than the dot.

I found some possible problems with other solutions, which would also apply to underlining the dot and the property together.

Error message Expected line break before '.foo'. is technically incorrect, because this rule expects a newline anywhere before the property (between the object and the property nodes), not necessarily before the dot. User can insert the newline after the dot:

/*eslint newline-per-chained-call: error*/

// lint-free
a.b().c().
    foo();

So it might be more correct to start underlining from the property node, like it already works in the actual version?

Also, this rule targets computed properties as well. These property nodes can be multiline, e.g.:

/*eslint newline-per-chained-call: error*/

// Expected line break before `[f({`
a().b()[f({
 prop: 1 
})]();

Should we, in this case, underline the whole property or maybe just the first token? Or perhaps only part of the property that is on the same line with the object, similar to how the error message works in the actual version (function getPropertyText).

[nzakas]

I think including the dot is fine as I’d expect we would include the opening bracket for computed properties. While the dot case message might not be 100%, I think it’s close enough and it seems most people like the break before the dot.

For long computed properties, I’d prefer to see the whole property underlined but only the first part in the actual error message.

[mdjermanovic]

I think including the dot is fine as I’d expect we would include the opening bracket for computed properties. While the dot case message might not be 100%, I think it’s close enough and it seems most people like the break before the dot.

For long computed properties, I’d prefer to see the whole property underlined but only the first part in the actual error message.

This should be done now.

Some examples:

[nzakas]

LGTM

[kaicataldo]

Thanks for contributing!