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

Enhance indentation of blocks #151

Merged
merged 4 commits into from
May 8, 2020
Merged

Conversation

edam
Copy link
Contributor

@edam edam commented Oct 2, 2019

Added lua-indent-nested-block-content-align var, which disables the aligning of content of nested blocks to the opening brace and indents normally. It defaults to t, which is the previous behaviour.

E.g. 1:

With lua-indent-nested-block-content-align set t (default), then:

call_some_fn( something, {
                val = 5,
                another = 6,
} )

Wth lua-indent-nested-block-content-align set nil, then:

call_some_fn( something, {
    val = 5,
    another = 6,
} )

E.g. 2:

With lua-indent-nested-block-content-align set t (default), then:

local def = {
  some_very_long_name = { fn =
                            function()
                              return true
                            end
  }
}

Wth lua-indent-nested-block-content-align set nil, then:

local def = {
  some_very_long_name = { fn =
      function()
        return true
      end
  }
}

@immerrr
Copy link
Owner

immerrr commented Oct 2, 2019

Hey, thank you for the contribution!

It has docs, maintains backward compability, and the change is very focused and self-contained. Great stuff! Could you please add some tests to ensure this behaviour is not lost in any potential refactoring down the road?

@kidd kidd mentioned this pull request Oct 14, 2019
@edam edam force-pushed the indent-nested-blocks branch from 1068951 to aee46da Compare November 18, 2019 20:37
@edam edam changed the title added lua-indent-nested-block-content-align var Enhance indentation of blocks Nov 18, 2019
@edam
Copy link
Contributor Author

edam commented Nov 18, 2019

I've also added lua-indent-close-paren-align var, to change the way the close parenthesis are aligned, so that they always align with the beginning of the line with the open parenthesis on. It defaults to t, which is the previous behaviour.

E.g.,

With lua-indent-close-paren-align set to t (the default), then:

local foo = setmetatable( {
    a = 4,
    b = 5,
                          }, {
    __index = some_func,
} )

With lua-indent-close-paren-align set to nil, then:

local foo = setmetatable( {
    a = 4,
    b = 5,
}, {
    __index = some_func,
} )

@immerrr
Copy link
Owner

immerrr commented Nov 19, 2019

Hey, awesome stuff with the tests, but I'm not sure if we need the close-paren variable: if you are not aligning the block, there is not much sense in aligning the closing brace.

@edam
Copy link
Contributor Author

edam commented Nov 26, 2019

The nested-block-content-align variable only affects the content of nested blocks. The lua-indent-close-paren-align variable does something different.

E.g.,

Default settings:

call_some_fn( something, {
                val = 5,
                val = 7,
                         }, {
                val = 6,
} )

With lua-indent-nested-block-content-align set to nil:

call_some_fn( something, {
    val = 5,
    val = 7,
                         }, {
    val = 6,
} )

With lua-indent-close-paren-align set to nil:

call_some_fn( something, {
                val = 5,
                val = 7,
}, {
                val = 6,
} )

With both lua-indent-nested-block-content-align and lua-indent-close-paren-align set to nil:

call_some_fn( something, {
    val = 5,
    val = 7,
}, {
    val = 6,
} )

@edam
Copy link
Contributor Author

edam commented Nov 26, 2019

Would you like me to combine lua-indent-nested-block-content-align and lua-indent-close-paren-align in to a single variable?

@immerrr
Copy link
Owner

immerrr commented Nov 27, 2019

Would you like me to combine lua-indent-nested-block-content-align and lua-indent-close-paren-align in to a single variable?

Hmm... I think I missed one case here.

My original idea stemmed from the fact that there could be different preferences w.r.t. where should the table content go:

  • either aligned to the opening brace plus indented (current behaviour in lua-mode)
  • or indented as the line of the opening brace plus one offset (the most popular suggested alternative)

But I couldn't find a reason for the closing brace to be anywhere else but at content indentation minus 1 indentation offset. This was about the case when there is no potential alignment inside the table, i.e. when the inside table elements start on the subsequent line,

foo(bar, {
    1,
    2,
})

But then your comment made me realize that there are still cases when the first element in the nested table is on the same row as the opening brace and it creates an alignment dilemma, so to speak, even without the outer function call:

-- this looks fine
foo = {1,
       2,
       3}

-- but the following two look strange
foo = {1,
       2,
       3
}

foo = {1,
       2,
       3
      }

-- although less strange than "minus one level of indentation"
foo = {1,
       2,
       3
  }

So I guess it would make sense to introduce a variable for people to "pick their poison" in cases when table elements are aligned, but the close brace is on a separate line.

@edam
Copy link
Contributor Author

edam commented Dec 6, 2019

So are you saying to leave it as it is, so the user has the choice? 🤔

@immerrr
Copy link
Owner

immerrr commented May 8, 2020

Sorry for the delay here.

I'm going to merge it. Thank you for your work and for your patience!

@immerrr immerrr merged commit a3a71b1 into immerrr:master May 8, 2020
@immerrr
Copy link
Owner

immerrr commented May 25, 2020

Hey!

Looks like lua-indent-nested-block-content-align setting applies not only to nested blocks, but to any alignment in general.

foobar(123,
   456)

In this example there are no nested blocks, but since alignment is disabled, 456 is indented instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants