Skip to content

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

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

Open
idbrii opened this issue Jan 16, 2022 · 9 comments
Labels
enhancement New feature or request rfc/in discussion This issue is currently in discussion, comments are welcome!

Comments

@idbrii
Copy link

idbrii commented Jan 16, 2022

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
Copy link
Author

idbrii commented Jan 16, 2022

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
Copy link

gegoune commented Jan 16, 2022

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

@JohnnyMorganz
Copy link
Owner

I'd suggest we preserve chain line breaking.

As I mention in #78 (comment), 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

@JohnnyMorganz JohnnyMorganz added enhancement New feature or request rfc/in discussion This issue is currently in discussion, comments are welcome! labels Jan 17, 2022
@idbrii
Copy link
Author

idbrii commented Jan 18, 2022

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
Copy link
Owner

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 #123, but there was an example which made this look quite odd: #123 (comment)

@idbrii
Copy link
Author

idbrii commented Jan 24, 2022

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
Copy link

cseickel commented Feb 6, 2022

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
Copy link
Contributor

uga-rosa commented May 14, 2023

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
Copy link
Author

idbrii commented Jun 10, 2023

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 #78 (comment), 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()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request rfc/in discussion This issue is currently in discussion, comments are welcome!
Projects
None yet
Development

No branches or pull requests

5 participants