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

Allow single line function definitions #248

Closed
echasnovski opened this issue Aug 29, 2021 · 18 comments · Fixed by #479
Closed

Allow single line function definitions #248

echasnovski opened this issue Aug 29, 2021 · 18 comments · Fixed by #479
Labels
rfc/in discussion This issue is currently in discussion, comments are welcome!

Comments

@echasnovski
Copy link

I think it would be useful to allow definitions of "simple" functions to be placed on single line. So that, for example, the code func(function() return true end, 2) will not be formatted as

func(function()
  return true
end, 2)

The notion of "simple function" might be aligned with "simple if" from #232. This is also somewhat related to #188.

@JohnnyMorganz
Copy link
Owner

Hm, this does seem like a reasonable request. I'm going to think about it, and its open for discussion

@JohnnyMorganz JohnnyMorganz added the rfc/in discussion This issue is currently in discussion, comments are welcome! label Aug 29, 2021
@JohnnyMorganz
Copy link
Owner

To add onto this further, it seems that prettier does do something similar for "simple" functions:

let foo = func(() => true, 2)

However, like in #161 for if statements, JavaScript/TypeScript has two different syntaxes to differentiate between these two (with braces, and without braces).
If we include braces in the JS version, prettier formats it like this:

let foo = func(() => {
  return true;
}, 2);

Since, in Lua, we have no differentiation between these two, we need to go all in for either one or the other style (same as with if statements). There are disagreements between which style is better here.

@echasnovski
Copy link
Author

I understand that adding another setting is a slippery slope, but this behavior can be controlled by something like 'collapse_simple_statements'. When set to true it tries to make "simple" statements occupy as least number of lines as possible. What makes a statement (if, function, maybe even do, etc.) "simple" is defined separately (something along the lines of #161).

@matthargett
Copy link

I'm curious what prettier does for

let foo = func(() => {}, 2)

I'm also curious if the formatting above has more to do with the JS-mandatory ; (which wouldn't be mandatory in Lua) more than the style of function expression.

@matthargett
Copy link

Verified with prettier 2.32:

let foo = func(() => {}, 2);

let foo = func(() => {
  return false;
}, 2);

let foo = func(() => false, 2);

let foo = func(() => [true, false, "boo"], 2);

what I'd expect the equivalent Lua 'native' code to do:

local foo = func(function() end, 2)

local foo2 = func(function() return false end, 2)

local foo3 = func(function() return false end, 2)

local foo4 = func(
  function() return true, false, "boo" end, 
  2
)

@echasnovski
Copy link
Author

I have a question about the fourth one. Do you expect it not to be one line because function returns multiple values? Because in one line it occupies 62 characters which should pass default width:

local foo4 = func(function() return true, false, "boo" end, 2)

Also a more Lua 'native' code might be:

local foo4 = func(function() return {true, false, "boo"} end, 2)

@JohnnyMorganz
Copy link
Owner

Apologies for the long delay on any update for this.
I was reminded in #161 (comment) the main reason why I don't think this is a great idea, namely:

Rodux.Store.new(function(state) return state, mockState end, nil)
Rodux.Store.new(function(state) return state end, mockState, nil)

On a single line, its much more difficult to realise that there is an issue here (the second version is correct, but if you imagine this in a larger codebase, and you glance over it, this isn't going to be immediately noticed).

In comparison with the second expanded version:

Rodux.Store.new(function(state)
    return state
end, mockState, nil)
Rodux.Store.new(function(state)
    return state, mockState
end, nil)

Now we can better see that we have a big difference here.

If we only applied this to functions with single returns, it isn't really better since we could have a case where version 1 above was actually the correct syntax, but we write version 2 - its just the same issue but inverted.

Because of this, I'm inclined to unfortunately say no to this

@echasnovski
Copy link
Author

Sure, I see where you are coming from. I still think that this might be a good candidate for a setting, as this edge case seems to be quite rare compared to overall benefits (smaller code base; more unified formatting when supplying anonymous function as argument).

Strangely enough, those two cases in one line form are immediately seen as different in your example here thanks to syntax highlighting (which is very common, I'd say).

@JohnnyMorganz
Copy link
Owner

JohnnyMorganz commented Dec 1, 2021

Strangely enough, those two cases in one line form are immediately seen as different in your example here thanks to syntax highlighting (which is very common, I'd say).

Haha yeah, as I was writing it I was thinking this too 😄, but normally (for this example) you wouldn't have these two lines next to each other in a real world codebase (probably even just only have one in a single [group of] file[s]), so the syntax highlighting difference wouldn't be visible.

I can still consider an option, but unfortunately I am very stubborn on adding new options, so it could be a while until anything comes about from it :P

@echasnovski
Copy link
Author

I can still consider an option, but unfortunately I am very stubborn on adding new options, so it could be a while until anything comes about from it :P

Sure, totally get that. Just saying that there will be a demand for this in Neovim world.

@clason
Copy link

clason commented Jan 3, 2022

I would like such an option, too, since there are an increasing number of situations in Neovim where you want to pass a function reference with arguments, which requires something like

function()
  require'foo'.bar( { baz = 'quux' })
end

This would go well together with #232, which covers the same (single statement) case for conditionals.

As in #330, this could be combined into a single option, e.g.,

collapse_single_statement = Never | FunctionOnly | ConditionalOnly | Always

(with the first being default).

@echasnovski
Copy link
Author

I am very sorry to complain again about this, but considered this might be a useful observation on the matter.

During writing code I quite often find myself adding --stylua: ignore before using "one line function". Mainly because it still results into more readable (for me) text and less code lines than without it. So there are many of these:

--stylua: ignore
func(function() return true end, 2)

And also this issue seems to be somewhat popular, judging by the number of positive upvotes here.

@JohnnyMorganz
Copy link
Owner

No problem at all, I appreciate everyone's viewpoint on the matter.

I continue to think about this, and at times I sometimes think about implementing it, and other times not 😆.

I think what I might do is add an option for this in another branch, and see the impact it has in real world use cases. But that is no guarantee that it will be added or not, just a test. I'll look into it over the weekend

@JohnnyMorganz
Copy link
Owner

Apologies for the delay, I got busy last weekend.
I've added an example implementation at #479

It can be installed using

cargo install --git https://github.com/JohnnyMorganz/StyLua.git --branch singleline

then setting collapse mode to "Functions" (--collapse-mode=functions argument or collapse_mode = "Functions" in .toml)

Please let me know if there are any issues with it, but as I mentioned earlier it is not guaranteed to be added.
The option has been added in a way to maybe also allow #161 as well, configured behind the option - if that is of interest, then let me know as well.

@echasnovski
Copy link
Author

Made a comment in PR.

Definitely interested also in one-ilne ifs. Option values from this comment might be a good way of configuring this?

@JohnnyMorganz
Copy link
Owner

JohnnyMorganz commented Jun 26, 2022

Added in support for one line ifs as well following this syntax:

This would go well together with #232, which covers the same (single statement) case for conditionals.

As in #330, this could be combined into a single option, e.g.,

collapse_simple_statement = Never | FunctionOnly | ConditionalOnly | Always

(with the first being default).

andreaswachowski added a commit to andreaswachowski/dotfiles that referenced this issue Oct 14, 2023
@theherk
Copy link

theherk commented Nov 17, 2023

Understanding this is closed, I'm still left thinking either I am missing where a clear explanation is found or it warrants being explained better. The fact that:

local _ = { function() somefunc() end }

being by default expanded to:

local _ = {
  function()
    somefunc()
  end,
}

is completely counterintuitive when the only mitigating setting is collapse_simple_statement. It seems odd that I have to request a statement that is already collapsed to be ... collapsed to prevent it from being expanded. I'd think there could be, in addition not in lieu of, a setting expand_simple_statement that defaults to true.

And that actually seems maybe even more terrible, but I've spent a bit of time trying to understand why I have to set a collapse setting to prevent an expansion. It is a strange situation.

I hope I'm being clear.

@JohnnyMorganz
Copy link
Owner

JohnnyMorganz commented Nov 19, 2023

So, there are a couple of reasons for this. Firstly, the collapse option was not originally intended but was added due to popular demand (meaning no other options were really considered at the time).

But mainly, one of the original goals for StyLua (similar to prettier or black) is to provide automated and consistent formatting. This means we do not rely on the input code style to determine the output code style, otherwise different starting points would produce different results. (The only place this is violated is for deciding whether to collapse tables if they were already expanded - prettier has a good discussion about it at https://prettier.io/docs/en/rationale#multi-line-objects, and also discusses the concept for "formatter reversibility")

A key reason to enforce consistency is the original principle to reduce bikeshedding. If we rely on input style, then we lose out on this: now, style nit comments become a thing again (e.g., a review comment that says "nit: please collapse this so that stylua leaves it collapsed")

The idea that you suggest would unfortunately break this goal, and I try to shy away from reliance on input as much as possible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants