jsx-indent rule autofixer

[eelyafi]

Due to limitations of the RuleFixer class there is no way to safely fix these two cases:

1. When a file uses spaces but options requires tab and vice-versa.
2. If a parent node didn't pass the rule, autofixer won't fix folded child nodes. Second run will actually fix the next child node.

Even with these limitations the autofixer found to be very useful and was already used on Pinterest codebase.

[ljharb]

Can you explain more about why you can't autofix to tabs?

[eelyafi]

Let's say we have "\t" and we want to replace "\t" with 4 spaces. In order to do so we have to run fixer.replace(). Unfortunately fixer.replace() won't let you increase number of characters.
Basically, if there are two spaces it won't replace them with more than two characters. On the other hand fixer won't let you run more than one command. Because of that there is no way to replace spaces with empty string first and then add needed amount of spacing.

[ljharb]

@eelyafi does that mean it's impossible to have a fixer for indent in eslint core?

[eelyafi]

@ljharb, after additional debugging I found that replaceTextRange will let you replace text. The actual reason why it won't replace one type of spacing with another is in getNodeIndent function.
The regExp in this function will look for spaces or tabs depending on the options passed in. Thus gotten variable will be always zero if file uses spaces and tabs are passed as an option.

[eelyafi]

Ping :)

[ljharb]

I'm very hesitant to merge an autofixer that doesn't fix all forms of the rule. If we can't support tabs, we shouldn't support spaces, and vice versa.

[eelyafi]

In reality many existing autofixers are far away from perfection. This autofixer will cover most of the cases and won't break code.

[ljharb]

Can this just be '\t' instead of the character code?

[eelyafi]

sure

[ljharb]

can we add a comment here detailing what cases are not covered? Ideally we'd also add skipped/commented-out test cases for that future work.

[eelyafi]

Addressed the comments

[ljharb]

LGTM pending a rebase

[eelyafi]

Rebased

[eelyafi]

@ljharb is it good to merge ?

[ljharb]

@eelyafi I'm waiting for one of the other contributors to review it.

[lencioni]

It would be good to note the limitations of the autofixer in the documentation, and also open an issue to improve it. Otherwise LGTM.

[eelyafi]

@lencioni I updated docs and created an issue #865

[eelyafi]

@ljharb @lencioni ping