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 using formatters for indentation adjustment when formatters are available #19847

Open
borekb opened this issue Feb 3, 2017 · 8 comments
Assignees
Labels
editor-autoindent Editor auto indentation issues under-discussion Issue is under discussion for relevance, priority, approach
Milestone

Comments

@borekb
Copy link

borekb commented Feb 3, 2017

VSCode 1.9 introduced initial support for line indenting as per the release notes, I think it's this commit: be1b5f7. I'm very interested in this feature, big 👍 for bringing it into VSCode.

Here is some initial feedback:

  • I tried for a couple of file formats, it only worked for TypeScript and not e.g., for JSON or PHP.
  • Formatting (Alt+Shift+F) can already do indentation better than the new implementation, maybe it could be used as a fallback if the language doesn't have proper indentation rules.
  • If I read the code correctly, it re-indents the whole file. It should respect my selection. For example, if I have only 10 lines selected, it should re-indent those. If I don't have any selection, it should do the whole file.
  • The command is currently editor.action.reindentlines, proper casing would be editor.action.reindentLines.
@rebornix
Copy link
Member

rebornix commented Feb 3, 2017

@borekb thanks for your detailed feedback!

The command is currently editor.action.reindentlines, proper casing would be editor.action.reindentLines

Yes it should be improved

If I read the code correctly, it re-indents the whole file. It should respect my selection. For example, if I have only 10 lines selected, it should re-indent those. If I don't have any selection, it should do the whole file.

The reason that right now we don't support Reindent in ranges is that we have some issues with boundaries in embedded-language files. Some of them are just TextMate Grammar issues but I'm sure if that can fix all. Once that issue is addressed, I think it's easy to support reindent in ranges/selection. And then Reindent when paste/moveLines/etc.

I tried for a couple of file formats, it only worked for TypeScript and not e.g., for JSON or PHP.

It's on the plan, adding Indentation Rules to most languages we support

Formatting (Alt+Shift+F) can already do indentation better than the new implementation, maybe it could be used as a fallback if the language doesn't have proper indentation rules.

Nice idea! I'm thinking about a similar solution like running formatting on indentation characters and see what's the proper edits a formatter would provide. That means we run format on leading whitespaces instead of whole lines but there might be performance hit while handling large files.


Really appreciate your feedback! Feedback and PRz are always welcome.

@rebornix rebornix added the editor label Feb 3, 2017
@borekb
Copy link
Author

borekb commented Feb 3, 2017

That means we run format on leading whitespaces instead of whole lines

Exactly, that's how I think about "reindenting" as a user. Technically, the two commands (formatting vs. indenting) should probably share the same logic.

@rebornix
Copy link
Member

rebornix commented Feb 6, 2017

Did some basic investigation on this and I found that leveraging formatter to do indent is kind of ideal as formatters' behaviors are not predictable. Take below code as example

foo() {
var a;
  }

I obverse following:

  • Format selection on leading whitepsaces doesn't work
  • Format } works
  • Format v on the second line doesn't work
  • Format var a; on the second line works

You can see that TypeScript formatter has no idea about adjusting the indentation if don't select any thing, even though it supports Format Selection.

We may need other ways to handle it as we can't force formatters to take care of indentation always. But it might be a good idea to allow formatters to provide a separate function about indentation adjustment. Any thoughts @mjbvz ?

@rebornix rebornix added the under-discussion Issue is under discussion for relevance, priority, approach label Apr 19, 2017
@borekb
Copy link
Author

borekb commented May 12, 2017

Any news on this?

@rebornix
Copy link
Member

@borekb sorry I don't have time working on this issue this iteration but it's on my radar, I received similar issues or feedback on this every other day

@dobriai
Copy link

dobriai commented May 14, 2017

FWIW: This feature is a life-saver for me. As much as I appreciate js-beautify, it seems that it cannot be told not to mess with my white-space. Trying to format something like this:

let aha          = 7;
let badahabadaha = 9;

mangles it, by trimming the white space in front of '= 7;' and thus looses the '=' alignment! Big deal and I just don't see a way to turn this behavior off.

Reindent Lines seems to do whatever most other editors do, so this is the cure for me. But it is very important to have a version that works on the current selection only!

@rebornix rebornix added the editor-autoindent Editor auto indentation issues label Jun 7, 2017
@alexdima alexdima removed the editor label Nov 23, 2017
@rebornix rebornix changed the title Reindent lines in 1.9 – feedback Explore using formatters for indentation adjustment when formatters are available Nov 3, 2020
@rebornix
Copy link
Member

rebornix commented Nov 3, 2020

The title is modified to one good idea proposed above: when there is a good formatter registered for a language, we should probably use the formatter other than regex based indentation rules.

@andycraig
Copy link

andycraig commented Nov 3, 2020

The title is modified to one good idea proposed above: when there is a good formatter registered for a language, we should probably use the formatter other than regex based indentation rules.

@rebornix A user of the vscode-R extension pointed out that the R formatter relies on the code being parseable. This means that Format Selection won't fix indentation if the selection only contains part of a for loop, for example. My understanding is that Reindent Selected Lines should still work in this case, because it uses simpler regex-based rules. So perhaps Reindent Selected Lines should use regex-based indentation rules, even if Reindent Lines uses the registered formatter? Or perhaps indentation should have its own LSP command, which the language server can implement or not?

(Reindent Selected Lines isn't working at all at the moment for R, but that's a separate issue.)

EDIT: I read this thread more carefully and noticed that I've basically just repeated what you observed in an earlier comment: #19847 (comment)

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 under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

6 participants