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

Turns readable multi-line function calls into harder to read, single line ones, but are under the column width #78

Open
Kampfkarren opened this issue Mar 13, 2021 · 14 comments · Fixed by #131
Labels
enhancement New feature or request hard Issue is difficult to solve rfc/in discussion This issue is currently in discussion, comments are welcome!

Comments

@Kampfkarren
Copy link

I'm not sure if this is something that's able to be fixed, but this is the before and after. I much prefer the before.

image

image

image

image

vvv This is 140 columns long, without any configuration.

image

@JohnnyMorganz
Copy link
Owner

I definitely also prefer the former styling here, so this should be looked into.

I think part of the reason for this problem is because of the way I set up formatting for function args - when it happens, StyLua only sees the parentheses and anything inside of them, not the previous information on the line - so it's hard to judge when to wrap. I'm probably going to have to give it access to some more context

@JohnnyMorganz JohnnyMorganz added the bug Something isn't working label Mar 13, 2021
@JohnnyMorganz
Copy link
Owner

Just thought I'd let you know - you may find that the formatting is better for you if you reduce the column with (maybe to something like 80 or 100). It may help in these situations, but there are still cases where it won't do as wanted/expected (such as your local assignment example with three variables - it won't catch that this line is greater than the column width.)

@Kampfkarren
Copy link
Author

Maybe, but 120 cols is already the rule I follow internally, feels weird to lower that down for the sake of StyLua.

@Stanzilla
Copy link

Yeah I ran into this as well, feels a bit weird having this tied to the col width only!

@JohnnyMorganz
Copy link
Owner

I was looking at this further - and I realise that the first four screenshots you posted - they are all under the column width. I can fix the last one, because that's over it, but I'm not sure what to do with the first few ones.

Sticking with column width as a rule, the first few ones are all valid. If I were to paste the same code into prettier (and change column width to 120), prettier would do the same thing here.

I do agree that the pre-formatting versions, they look nicer. But I'm not entirely sure what kind of heuristic to apply here to get something like that, whilst keeping the column width set to 120.

@JohnnyMorganz JohnnyMorganz reopened this Apr 30, 2021
@JohnnyMorganz JohnnyMorganz added enhancement New feature or request rfc/in discussion This issue is currently in discussion, comments are welcome! and removed bug Something isn't working labels Apr 30, 2021
@JohnnyMorganz JohnnyMorganz changed the title Turns readable multi-line function calls into harder to read, single line ones Turns readable multi-line function calls into harder to read, single line ones, but are under the column width Apr 30, 2021
@SafeteeWoW
Copy link

Function call which contains long concatenated string arguments are turned into single line. Can this be fixed?

For example

error("11111111111111111111111111111111111111111111111"
 .. "11111111111111111111111111111111111111111111111111111111")

I want this to be 2 lines to be fit in 120 columns, rather than a single line of

error("11111111111111111111111111111111111111111111111" .. "11111111111111111111111111111111111111111111111111111111")

@JohnnyMorganz
Copy link
Owner

Function call which contains long concatenated string arguments are turned into single line. Can this be fixed?

For example


error("11111111111111111111111111111111111111111111111"

 .. "11111111111111111111111111111111111111111111111111111111")

I want this to be 2 lines to be fit in 120 columns, rather than a single line of


error("11111111111111111111111111111111111111111111111" .. "11111111111111111111111111111111111111111111111111111111")

I have opened a new issue for this, as it is slightly different to this issue: #139

@JohnnyMorganz JohnnyMorganz added the hard Issue is difficult to solve label Jun 14, 2021
@idbrii
Copy link

idbrii commented Jan 16, 2022

Demonstrations using column_width

While there's agreement that column_width isn't a solution, I was trying to figure out how to use it and thought I'd document my findings and provide some sample code for testing.

Given some code in the OP:

HealthRegen.ValidateInstance = t.intersection(
	ComponentUtil.HasComponentValidator("Health"),
	ComponentUtil.HasComponentValidator("Target")
)

return function(...)
	callback(
		LOG_FORMAT:format(
			os.date("%H:%M:%S"),
			key,
			level,
			fmt.fmt(...)
		)
	)
end

I have to drop down to --column-width=70 to get the callback to split. At 80, it was still one line.

But at 70 then it can get overly aggressive:

fh:write(n_tests, " tests (", n_passed, " ok, ", n_tests - n_passed, " failed, ", n_errors, " errors)\n")
-- what would be left alone at 120 becomes
fh:write(
    n_tests,
    " tests (",
    n_passed,
    " ok, ",
    n_tests - n_passed,
    " failed, ",
    n_errors,
    " errors)\n"
)

@idbrii
Copy link

idbrii commented Jan 16, 2022

Seems like the current rule is: if a function call joined to a single line would be over the column limit, then break it on each argument (comma).

Proposal

Change the rule to take the current state into account:

  • if a function call is currently broken after each argument
  • or if a function call joined to a single line is over the column limit
  • then break it on each argument.

However, I'm not sure how you'd handle cases where the input doesn't break on each arg, but only on some args:

fh:write(
    n_tests, " tests (",
    n_passed, " ok, ",
    n_tests - n_passed, " failed, ",
    n_errors, " errors)\n"
)

return function(...)
    callback(
        LOG_FORMAT:format(
            os.date("%H:%M:%S"),
            key, level,
            fmt.fmt(...)
        )
    )
end

I think, only break lines that are over the limit?

Alternatively, handle function calls where the first line ends with a ( differently from ones that end with an arg:

LOG_FORMAT:format(os.date("%H:%M:%S"),
    key, level,
    fmt.fmt(...)
    )
-- becomes (single line under column-width) 
LOG_FORMAT:format(os.date("%H:%M:%S"), key, level, fmt.fmt(...))
LOG_FORMAT:format(
    os.date("%H:%M:%S"),
    key, level,
    fmt.fmt(...)
    )
-- becomes
LOG_FORMAT:format(
    os.date("%H:%M:%S"),
    key,
    level,
    fmt.fmt(...)
)

But a function that when single-lined would be beyond column-width always becomes the second style.

@JohnnyMorganz
Copy link
Owner

Change the rule to take the current state into account:

  • if a function call is currently broken after each argument

I'm a bit wary about relying on existing input state like this. It leads to a few issues, and is essentially a "hidden" option.
We do have something similar for tables, but it also has the same formatter reversibility issue (also described by prettier https://prettier.io/docs/en/rationale.html#%EF%B8%8F-a-note-on-formatting-reversibility):

What does reversible mean? Once an object literal becomes multiline, Prettier won’t collapse it back. If in Prettier-formatted code, we add a property to an object literal, run Prettier, then change our mind, remove the added property, and then run Prettier again, we might end up with a formatting not identical to the initial one. This useless change might even get included in a commit, which is exactly the kind of situation Prettier was created to prevent.

If we rely on the input state (looking for a newline between the first brace and argument), then if you add another argument which goes over the column width, it gets expanded. If you then delete this argument (where it would now be under the column width), you are still expanded, and always will be until you explicitly remove the newline. This reversibility issue is a problem (which can lead to unnecessary diffs too), and something which we want to try and avoid.

@idbrii
Copy link

idbrii commented Jan 18, 2022

Feels like this issue is "humans know better how to make this understandable" running up against purely syntactic formatting. (Similarly, I'd prefer hidden options to adding --stylua: ignore annotations.) Regardless, stylua's opinionated stance is a design goal so I can see how more options deviates from that goal.

@TjeuKayim
Copy link

For the input below, I expected that the formatted output kept the same amount of lines:

foo.bar('-------------------------------------------------------------------------------------------------------------')
	.returns()
foo.bar('--------------------------------------------------------------------------------------------------')
	.returns()
	.way()
	.longer()
	.chain()

However, StyLua formatted it like the output below, where each function call chain is joined on a line. The lines are longer than the column limit of 120, and that doesn't read nice.

foo.bar("-------------------------------------------------------------------------------------------------------------").returns()
foo.bar("--------------------------------------------------------------------------------------------------").returns().way().longer().chain()

I'm posting it here, because it looks like a duplicate of this issue.

@afknst
Copy link

afknst commented Feb 13, 2022

I run into this issue recently.
Could you please just add an option to keep line breaks (for non-empty lines)?
This way users can fix the case manually.

@afknst
Copy link

afknst commented Feb 13, 2022

I have some code like bellow;

-- A
a = A()

-- B
.B()

-- C
.C()

And after several runs, the formatter makes it like this:

-- A
a = A() -- B.B() -- C.C()

This is intolerable since the code does totally different things after being formatted.

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

Successfully merging a pull request may close this issue.

7 participants