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

Explore indentation issues with the indentation test tool #209863

Open
aiday-mar opened this issue Apr 8, 2024 · 2 comments
Open

Explore indentation issues with the indentation test tool #209863

aiday-mar opened this issue Apr 8, 2024 · 2 comments
Assignees
Labels
editor-autoindent Editor auto indentation issues exploration

Comments

@aiday-mar
Copy link
Contributor

This issue has been created to track indentation issues that have been found through the usage of the indentation test tool. The indentation test tool applies the reindent lines operation on consecutive pairs of lines on well indented/formatted code, and finds the cases when the suggested indentation differs from the existing indentation. This tool can be run on large repos in order to quickly find cases when the indentation is incorrect.

@aiday-mar aiday-mar added editor-autoindent Editor auto indentation issues exploration labels Apr 8, 2024
@aiday-mar aiday-mar self-assigned this Apr 8, 2024
@aiday-mar
Copy link
Contributor Author

aiday-mar commented Apr 9, 2024

The indentation test tool is introduced in the following PR: #209928. It takes in a directory as input, runs over all the TS files (for example), applies the reindentation and writes back the files. Since the files are already properly indented/formatted, edits proposed the reindentation operation correspond presumably to incorrect indentation edits.

This comment will be used to record the findings from running the tool on different repos.

Repo: VS Code

  • Incorrect reindentation of block comments:

Screenshot 2024-04-09 at 11 53 10

  • Incorrect indentation when the if/for/while statements contain a string in the signature (see code-import-patterns.ts file)

Screenshot 2024-04-09 at 11 56 34

When the following is finished (#209862), this should no longer be an issue.

  • When defining several variables over multiple lines with the same const keyword, there is incorrect indentation as follows (code-no-unused-expressions.ts):
Screenshot 2024-04-10 at 15 50 27

This happens because we do not have a pattern to increase the indentation after a comma after a const statement. We would like to decrease the indentation when the assignments are finished. We can not easily detect when to decrease the indentation. Link to pairs of indent/outdent pairs idea.

indentNextLinePattern is not enough here, because indentation should apply over several lines following the comma.

Another similar example is (bundle.ts)

Screenshot 2024-04-10 at 16 27 11
  • After following line return ... && we want to indent the next several lines until the end of the return statement. This currently does not work. As in the previous point how to detect when the indentation should be decreased. (code-no-unused-expressions.ts)
Screenshot 2024-04-10 at 15 51 01

The following is a similar case:

Screenshot 2024-04-10 at 16 06 54

As well as:

Screenshot 2024-04-10 at 16 07 25

As well as:

Screenshot 2024-04-10 at 16 23 23

As well as (extensions.ts)

Screenshot 2024-04-10 at 16 29 14
  • Indentation is decreased when the current line contains an opening brace as well as a string preceding it:
Screenshot 2024-04-10 at 16 05 06

This happens because our decrease indentation pattern contains a check on string which will no longer be needed when we will process the strings before appliyng the indentation patterns.

  • Incorrect indentation in cases where there should be double deindentation (snapshot.ts):
Screenshot 2024-04-10 at 16 16 41
  • The indentation rules do not perform indentation inside of the switch statements. Indentation is still correct in practise because the following case is followed by onEnter rules (testOutputScanner.ts)
Screenshot 2024-04-10 at 16 19 19
  • Indentation of tokens inside of a string:
Screenshot 2024-04-10 at 16 31 53
  • Incorrect indentation when there are nested braces after an opening brace. Consider the following case when you find pairs () after an ( of an if statement (treeshaking.ts):
Screenshot 2024-04-10 at 16 34 34
  • Indentation of current line should not take into account previous lines if they contain a string (util.ts)
Screenshot 2024-04-10 at 16 40 09
  • Incorrect indentation when array contains strings:
Screenshot 2024-04-10 at 16 41 11

Should be fixed by #209862

Screenshot 2024-04-10 at 16 49 47
  • Indentation should not be increased, if it already has been increased previously. Consider the following case for balance.ts file:
Screenshot 2024-04-10 at 16 51 14

Indentation should not be increased because it already has previously been increased through indentation of if statement.

In a similar manner, indentation should be decreased in the following case but is not:

Screenshot 2024-04-10 at 16 54 59
  • Indentation should be reset at the end of a statement that is indented:
Screenshot 2024-04-10 at 16 59 17

Consider the case above. The if statement on line 31 should be on the same indentation level as line 28.

  • Code inside of strings should not be indented:
Screenshot 2024-04-10 at 17 03 10

When taking into account indentation of multi-line comments:

git diff --shortstat:
  4734 files changed, 32778 insertions(+), 32778 deletions(-)

When ignoring the incorrect indentation of multi-line comments:

git diff --shortstat:
  1647 files changed, 13842 insertions(+), 13842 deletions(-)

@aiday-mar
Copy link
Contributor Author

aiday-mar commented Apr 9, 2024

After some analysis it appears that the Reindent Lines command does not respect the onEnter rules. Consider the following case, the Reindent Lines command returns the code inside of the if statement on the same level as the if statement itself, but on pressing Enter after the curly open brace, we get the correct indentation (see file code-import-patterns.ts).

Screenshot 2024-04-09 at 12 05 28

An issue has been opened here: #209938

More specifically, if the Reindent Lines command is made to adopt the onEnter rules, we will no longer see the incorrect indentation with the JS Doc comments for example. We do not want to adopt the onEnter rules right now however because we need to use the current code to find indentation issues caused by the regexes that could be eclipsed by the onEnter rules. So this will be done at the end of the improvement cycle.

In the meantime, a new tool will be developed, the indentation onEnter test tool which will test that the onEnter rules work correctly independently of the general indentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editor-autoindent Editor auto indentation issues exploration
Projects
None yet
Development

No branches or pull requests

1 participant