JohnnyMorganz / StyLua

An opinionated Lua code formatter
Mozilla Public License 2.0
1.6k stars 72 forks source link

Multi-line chained functions are merged onto a less-readable single line, but under the column width #339

Open idbrii opened 2 years ago

idbrii commented 2 years ago

Related to #78, chained functions are merged onto a single line which makes them less readable -- even if a short line.

Before

-- under column-width
if do_icon then
    local icon_widget = tab:AddChild(Image(IMAGES.icon))
        :SetColor(COLORS.TEXT_LIGHT_ON_DARK)
        :SetSize(iconSize, iconSize)
end

-- over column-width
if do_icon then
    local icon_widget = tab:AddChild(Image(IMAGES.icon))
        :SetColor(COLORS.TEXT_LIGHT_ON_DARK)
        :SetSize(iconSize, iconSize)
        :SetStyle("intermediate-grey")
end

After stylua test.lua

-- under column-width, but far less readable
if do_icon then
    local icon_widget = tab:AddChild(Image(IMAGES.icon)):SetColor(COLORS.TEXT_LIGHT_ON_DARK):SetSize(iconSize, iconSize)
end

-- over column-width
if do_icon then
    local icon_widget = tab
        :AddChild(Image(IMAGES.icon))
        :SetColor(COLORS.TEXT_LIGHT_ON_DARK)
        :SetSize(iconSize, iconSize)
        :SetStyle("intermediate-grey")
end

Similar to my proposal on 78, I'd suggest we preserve chain line breaking. Or maybe an option to always break before function chaining?

idbrii commented 2 years ago

Another idea is to always break when a function is chained directly to a previous one. So always split between ): (in SetColor():SetSize()), but use line width to determine whether to split between letters and : (in tab:AddChild()).

Although, I'd generally prefer not to split tab:AddChild() unless it is itself a long line (ignoring chaining) -- this is the style in my Before.

gegoune commented 2 years ago

Another option could be not breaking on single : if under column-width, but always breaking if there are more :?

JohnnyMorganz commented 2 years ago

I'd suggest we preserve chain line breaking.

As I mention in https://github.com/JohnnyMorganz/StyLua/issues/78#issuecomment-1014688702, I want to try and avoid relying on input code to determine how to format something.

I do agree though that it does make it less readable though, so might be something to improve.

Another option could be not breaking on single : if under column-width, but always breaking if there are more :?

This is an interesting suggestion, and is probably a good alternative to relying on the input. Might be something to look into

idbrii commented 2 years ago

Another option could be not breaking on single : if under column-width, but always breaking if there are more :?

That's close to what I was thinking with my second comment, but should self:Func() args get broken too?

Given:

local icon_widget = tab
    :AddChild(ImageStore:Get(IMAGES.icon):Large())
    :SetColor(Color:Get(TEXT_LIGHT_ON_DARK))
    :SetSize(iconSize, iconSize)

I think break if multiple : would result in:

local icon_widget = tab
    :AddChild(ImageStore
        :Get(IMAGES.icon)
            :Large())
    :SetColor(Color
        :Get(TEXT_LIGHT_ON_DARK)) -- does it treat arguments as its own line?
    :SetSize(iconSize, iconSize)

But break every ): would give:

local icon_widget = tab:AddChild(ImageStore:Get(IMAGES.icon)
        :Large())
    :SetColor(Color:Get(TEXT_LIGHT_ON_DARK))
    :SetSize(iconSize, iconSize)
JohnnyMorganz commented 2 years ago

I think break if multiple : would result in:

local icon_widget = tab
  :AddChild(ImageStore
      :Get(IMAGES.icon)
          :Large())
  :SetColor(Color
      :Get(TEXT_LIGHT_ON_DARK)) -- does it treat arguments as its own line?
  :SetSize(iconSize, iconSize)

This is true, I didn't consider nested chains within calls - I'm not sure I like it so expanded like this.

But break every ): would give:

I assume this means like leave the first chain intact, but not the rest? This is a interesting idea, we actually discussed it earlier in https://github.com/JohnnyMorganz/StyLua/issues/123, but there was an example which made this look quite odd: https://github.com/JohnnyMorganz/StyLua/issues/123#issuecomment-821883893

idbrii commented 2 years ago

What if our rule was "break every ): or if a : call is multi line."?

That would maintain the format of my previous examples and match the format of the "good" example on 123. "is multi line" seems like a significant enough case to justify specialized behaviour.

cseickel commented 2 years ago

I think the ideal option for me would be to have an option to only change the line if it is over the specified column width. So it would introduce line breaks if it is too long like it does now, but never join lines if someone went through the trouble of formatting them as multi-line statements.

uga-rosa commented 1 year ago

What is the status of this proposal? I am in favor of it. Could you add a rule that if there is already a line break between ):, then follow it and break lines so that they become multiple lines, even if they are under column-width. Because the current handling of table is pretty close to the behavior I want. That is, the following formatting.

-- From
local a = {
  "a", "b", "c" }
-- To
local a = {
  "a",
  "b",
  "c",
}

-- No change in this one
local a = { "a", "b", "c" }
idbrii commented 1 year ago

Could you add a rule that if there is already a line break between ):, then follow it

I don't think that's likely:

As I mention in https://github.com/JohnnyMorganz/StyLua/issues/78#issuecomment-1014688702, I want to try and avoid relying on input code to determine how to format something.

However, I think the heuristic "break every ): or if a : call is multi line" should have good results:

somePromise
    :andThen(function()
        local x = 1
        local foo = "bar"
        return true
    end)
    :catch(function()
        local err = foobar
        print(err)
    end)
    :finally(function()
        doCleanup(someArg)
    end)

local icon_widget = tab:AddChild(ImageStore:Get(IMAGES.icon)
        :Large())
    :SetColor(Color:Get(TEXT_LIGHT_ON_DARK))
    :SetSize(iconSize, iconSize)

self:GetOwnedVehicles()
    :andThen(...)
    :catch(...)

items:modify()
    :andThen()
    :catch()

Compared to the current (0.17.1) behaviour:

somePromise
    :andThen(function()
        local x = 1
        local foo = "bar"
        return true
    end)
    :catch(function()
        local err = foobar
        print(err)
    end)
    :finally(function()
        doCleanup(someArg)
    end)

local icon_widget = tab:AddChild(ImageStore:Get(IMAGES.icon):Large())
    :SetColor(Color:Get(TEXT_LIGHT_ON_DARK))
    :SetSize(iconSize, iconSize)

self:GetOwnedVehicles():andThen(...):catch(...)

items:modify():andThen():catch()