config: migrate indent to stylistic

[MatheusBaldi]

Depends on:

#111
#115

Description

Base partial doen’t specify typescript specific rules, so the indent rule didn’t support typescript syntax. Now that we use the unified stylistic eslint plugin, this rule correctly support ts and js syntaxes by default, so some changes in behavior may be noticed.
Since this rule was defined using only eslint, not typescript-eslint, in some cases it was not being validated.

Also, this rule is known to be broken (currently, the docs reference this same issue: https://eslint.style/rules/default/indent#ts-indent), even more for ts syntax.

For those reasons, some places may start to raise errors. but most of them are expected.

[MrMarCode]

Could you clarify, please, on the phrase "Base partial doesn’t specify typescript specific rules, so the indent rule didn’t support typescript syntax"? It's true that the base partial doesn't define anything specific to typescript, but as soon as a developer includes the recommended typescript-eslint config, eslint picks up on the specific settings for the indent rule while linting typescirpt files. For example, in this project if you add a typescript file with 4 space indentation and run eslint, it will throw an error. However if you remove ...typescriptESLint.configs.recommended from index.js the errors go away. So evidently the settings we have are currently respected by typescript-eslint if the recommended rules are included in the config.

Are you saying that since typescript is automatically supported by stylistic, projects which previously did not include ...typescriptESLint.configs.recommended will now lint typescript files? Or are there other edge cases I'm missing here? Please feel free to give examples to help me understand.

Thanks for working through these changes and for this research! 🙂

[MrMarCode]

Since these other rules will also be migrated to stylistic, do we need to move them to a separate section? I wonder if we could keep all the rules in the same spot and just rename them? This would make it easier to review and make the git history cleaner.

[MatheusBaldi]

@MrMarCode Moving them is more about creating a separation within the file between which rules are from the stylistic package, and hence, only related to code formatting or style, and not logical aspects of the code and which are not. The code looks cleaner afterwards.
Your suggestion also simplifies the merging process for incoming PRs, thanks. Just a question though, is your suggestion about moving these rules in a separate commit in the last PR dedicated to it, or not doing it at all?

[MrMarCode]

Nice point, eslint separates logical rules from style rules. Additionally, we group our own rules from @silvermine together.

In this case though, my suggestion would still be to rename the rules and leave them in the same spot. Then at the end we could decide whether to move them all to the same spot in a separate PR, or if most of them are migrated to stylistic we could just move the logical rules to the end instead. Either way, doing it at the end will help avoid merge conflicts and make things easier to review.

[MatheusBaldi]

Could you clarify, please, on the phrase "Base partial doesn’t specify typescript specific rules, so the indent rule didn’t support typescript syntax"? It's true that the base partial doesn't define anything specific to typescript, but as soon as a developer includes the recommended typescript-eslint config, eslint picks up on the specific settings for the indent rule while linting typescirpt files. For example, in this project if you add a typescript file with 4 space indentation and run eslint, it will throw an error. However if you remove ...typescriptESLint.configs.recommended from index.js the errors go away. So evidently the settings we have are currently respected by typescript-eslint if the recommended rules are included in the config.

Are you saying that since typescript is automatically supported by stylistic, projects which previously did not include ...typescriptESLint.configs.recommended will now lint typescript files? Or are there other edge cases I'm missing here? Please feel free to give examples to help me understand.

Thanks for working through these changes and for this research! 🙂

Stylistic rules are not present in typescript-eslint recommended, so indent is not enabled for TS syntax here.

I've just tested adding indent to the typescript.js partial in a codebase where silvermine/eslint-config is used, and many indentation errors started to pop up. All these errors happen when related to TS specific sintax.

I've also tested removing the recommended typescript from silvermine/esling-config to see what would happen, and what I could notice is that, when applied, the indent rule used was the eslint one, not the typescript-eslint's. But TS syntax is ignored when I indent in a way that should raise an error. I'd say indent rule was either, not being applied to TS syntax or if it was, it would happen only in some cases. For parts of TS files where the syntax is only JS the indent rule works, but it ignores TS specific code.
This explains why some files were not correctly indented before in codebases depending on silvermine/eslint-config for indent rule. (I've checked, all occurrences of missed wrong indentations are for TS specific syntax like: interface properties, enum members, type properties and others)

Could you clarify, please, on the phrase "Base partial doesn’t specify typescript specific rules, so the indent rule didn’t support typescript syntax"?

Yes that's it. What I intended when I mentioned the base.js partial is that it would not enforce indent rule in TS syntax when used alone. But now, after these tests we see that even when using the entire silvermine/eslint-config through the index file, this is not applied to TS specific syntax.

For example, in this project if you add a typescript file with 4 space indentation and run eslint, it will throw an error

Could you verify if the sintax is typescript specific? These are some of the situations the wrong indentation were not detected in my tests interfaces properties, enum members, type properties, multiline variable definitions with type, properties passed to functions with a generic return type defined.

However if you remove ...typescriptESLint.configs.recommended from index.js the errors go away

Only removing this line will break the execution of eslint (when npm run eslint), which may cause the impression it is not raising indent errors in your editor. But once you fix the problems, indent errors start appearing again for plain js syntax.

[MrMarCode]

Nice research! I think this stems from me misunderstanding your comment earlier. For whatever reason when I read "typescript syntax" I thought you meant "typescript files." But it's clear to me now: the new stylistic rule lints typescript syntax. Even though the old indent rule does lint typescript files (according to our configuration), it did not lint typescript specific syntax within those typescript files.

Nice research and thanks for testing all these edge cases! 🙂

[MrMarCode]

LGTM once #115 is merged! Thanks Matheus

[MrMarCode]

LGTM!