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.

Sign up for GitHub

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

Jump to bottom

Avoid extra indentation on Go switches #6817

Merged
merged 1 commit into from
Apr 25, 2023
Merged

Avoid extra indentation on Go switches #6817

merged 1 commit into from
Apr 25, 2023

Conversation

mvdan
Copy link
Contributor

@mvdan mvdan commented Apr 19, 2023

(see commit message)

Fixes #6772.

the-mikedavis
the-mikedavis previously approved these changes Apr 19, 2023
View reviewed changes
@the-mikedavis the-mikedavis added the A-language-support Area: Support for programming/text languages label Apr 19, 2023
@pascalkuthe
Copy link
Member

@Triton171 I am not an expert on the indent queries but from your comment on #6772 it sounds like you had something slightly different in mind.

@Triton171
Copy link
Contributor

Yeah, this is not what I meant. I believe this issue needs essentially the same fix as #5713. This PR currently breaks go indentation in some ways (for example since the @outdent query for } is completely removed, instead of disabling it just for switch statements). @mvdan let me know if you have any questions on how to approach this.

@mvdan
Copy link
Contributor Author

mvdan commented Apr 20, 2023

I indeed had a different fix in mind at first, but then I realized that indenting on switches and selects is the wrong approach to begin with, since those aren't indented in Go at all - only their cases. That is what I tried to explain in the commit message :)

I have tested these changes locally and tried to find edge cases where they break (by using o at particular lines in files with switches, selects, etc), and it seems to me like indentation in Go works well now. @Triton171 if you found cases where this change breaks indentation, could you share details so I can reproduce?

I'm slightly confused by the need for the @outdent bits; I can add } there, or remove them all, and the indentation still works well for me. My best guess is that the @indent for nodes like block or communication_case already unindent when the nodes are closed, so I suspect that an extra @unindent is deduplicated.

@Triton171
Copy link
Contributor

Sorry, I should have been more specific. Removing the @indent for type_switch_statement and expression_switch_statement should be correct. However, the @outdent query for } is still needed e.g. for functions:

func f() {
    do_smt()
} // This curly brace is part of the function block but it shouldn't be indented, so it needs an @outdent query

Your change causes the last curly brace to be indented one level which is incorrect. This can be fixed by keeping the query but adding more exceptions to it:

(
    (_ "}" @outdent) @outer
    (#not-kind-eq? @outer "select_statement")
    (#not-kind-eq? @outer "type_switch_statement")
    (#not-kind-eq? @outer "expression_switch_statement")
)

Also, your solution will not correctly indent incomplete case expressions:

func f() {
    switch {
    case true: // Pressing <Enter> here will not increase indentation but it should
    }
}

This happens because the expression_case node is "too short" (i.e. it doesn't include the following whitespace). It can be fixed by adding @extend queries, like the one that was already there:

[
    (communication_case)
    (expression_case)
    (default_case)
    (type_case)
] @extend

I haven't tested all of this, so let me know if you encounter any issues.

@mvdan
Copy link
Contributor Author

mvdan commented Apr 20, 2023

Thank you, you're right that this PR as-is is not entirely correct. I spotted two of the problems you mentioned on my second day of using the file as I work. I'll come back with a new version :)

@mvdan
Copy link
Contributor Author

mvdan commented Apr 20, 2023

Your suggested changes do seem to do a lot better. I've updated the PR to include them.

I spotted two more bugs, starting at one level of indentation:

  1. switch {<Enter> (or the same with select) leaves you with an indented new line, when it shouldn't be:
    switch {
        // cursor; too much indentation
    }
  1. [<Enter>, which arguably doesn't make sense as Go syntax, leaves you with:
    [
        // cursor
] // too much outdentation

The second one is arguably not worth fixing, as I only found it by chance, but the first one we should fix. I'm not sure why I'm getting the indentation; switches and selects don't appear in any @indent or @extend.

@Triton171
Copy link
Contributor

Loogs good to me, thanks for working on this! I think the CI failure is unrelated (rebasing on the latest master should fix it).

  1. This is due to the auto-pairing code: Inserting a new line between matching parentheses will insert an additional line with an extra level of indentation. Fixing this is not that easy (it requires significant code changes and cannot be fixed by changing the queries), so I think it's fine for now. In the future, we could maybe run 2 indent queries (one for the closing parenthesis and one for the additional new line).
  2. I agree that this is not worth fixing unless this is something that frequently happens when editing code.

pascalkuthe
pascalkuthe previously approved these changes Apr 21, 2023
View reviewed changes
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM please rebase on master so the ci is fixed and we can merge

@pascalkuthe pascalkuthe added this to the 23.04 milestone Apr 25, 2023
@mvdan
runtime: avoid extra indentation on Go switches
d5a0f1a 
Unlike other languages, in Go, switches themselves are not indented;
it's just each case body which is indented by one level:

   switch foo {
   case "bar":
      baz()
   }

As such, we shouldn't @indent for type_switch_statement nor
expression_switch_statement, as otherwise inserted lines show up as:

   switch foo {
      // inserted with "o"
   case "bar":
         // inserted with "o"
      baz()
   }

With the fix, the inserted lines are indented properly:

   switch foo {
   // inserted with "o"
   case "bar":
      // inserted with "o"
      baz()
   }

I also verified that indentation on selects similarly works well.

Thanks to Triton171 for helping with this fix.

Fixes helix-editor#6772.
@mvdan mvdan force-pushed the go-indents branch 2 times, most recently from e8cefce to d5a0f1a Compare April 25, 2023 21:21
@mvdan
Copy link
Contributor Author

mvdan commented Apr 25, 2023

@Triton171 thanks again - that makes sense. I've attributed you in the commit message.

@pascalkuthe rebased.

@the-mikedavis the-mikedavis changed the title runtime: avoid extra indentation on Go switches Avoid extra indentation on Go switches Apr 25, 2023
@the-mikedavis the-mikedavis merged commit e7f25d8 into helix-editor:master Apr 25, 2023
@mvdan mvdan deleted the go-indents branch April 30, 2023 12:58
Triton171 pushed a commit to Triton171/helix that referenced this pull request Jun 18, 2023
@mvdan @Triton171
Avoid extra indentation on Go switches (helix-editor#6817)
1013057 
Unlike other languages, in Go, switches themselves are not indented;
it's just each case body which is indented by one level:

   switch foo {
   case "bar":
      baz()
   }

As such, we shouldn't `@indent` for type_switch_statement nor
expression_switch_statement, as otherwise inserted lines show up as:

   switch foo {
      // inserted with "o"
   case "bar":
         // inserted with "o"
      baz()
   }

With the fix, the inserted lines are indented properly:

   switch foo {
   // inserted with "o"
   case "bar":
      // inserted with "o"
      baz()
   }

I also verified that indentation on selects similarly works well.

Thanks to Triton171 for helping with this fix.
wes-adams pushed a commit to wes-adams/helix that referenced this pull request Jul 4, 2023
@mvdan @wes-adams
Avoid extra indentation on Go switches (helix-editor#6817)
d12c303 
Unlike other languages, in Go, switches themselves are not indented;
it's just each case body which is indented by one level:

   switch foo {
   case "bar":
      baz()
   }

As such, we shouldn't `@indent` for type_switch_statement nor
expression_switch_statement, as otherwise inserted lines show up as:

   switch foo {
      // inserted with "o"
   case "bar":
         // inserted with "o"
      baz()
   }

With the fix, the inserted lines are indented properly:

   switch foo {
   // inserted with "o"
   case "bar":
      // inserted with "o"
      baz()
   }

I also verified that indentation on selects similarly works well.

Thanks to Triton171 for helping with this fix.
@triallax triallax mentioned this pull request Oct 23, 2023
Closed
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
@mvdan
Avoid extra indentation on Go switches (helix-editor#6817)
5d12338 
Unlike other languages, in Go, switches themselves are not indented;
it's just each case body which is indented by one level:

   switch foo {
   case "bar":
      baz()
   }

As such, we shouldn't `@indent` for type_switch_statement nor
expression_switch_statement, as otherwise inserted lines show up as:

   switch foo {
      // inserted with "o"
   case "bar":
         // inserted with "o"
      baz()
   }

With the fix, the inserted lines are indented properly:

   switch foo {
   // inserted with "o"
   case "bar":
      // inserted with "o"
      baz()
   }

I also verified that indentation on selects similarly works well.

Thanks to Triton171 for helping with this fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pascalkuthe pascalkuthe pascalkuthe approved these changes

@the-mikedavis the-mikedavis the-mikedavis approved these changes

Assignees
No one assigned
Labels
A-language-support Area: Support for programming/text languages
Projects
None yet
Milestone
23.05
Development

Successfully merging this pull request may close these issues.

Extra indentation in Go switches
4 participants
@mvdan @pascalkuthe @Triton171 @the-mikedavis