Make default nesting in SciML style more readable

[ranocha]

After https://github.com/domluna/JuliaFormatter.jl/pull/729, the default nesting in the SciML style of functions with many arguments became hard to read, e.g.,

function alg_cache(alg::FineRKN4, u, rate_prototype, ::Type{uEltypeNoUnits},
    ::Type{uBottomEltypeNoUnits}, ::Type{tTypeNoUnits}, uprev, uprev2, f, t,
    dt, reltol, p, calck,
    ::Val{true}) where {uEltypeNoUnits, uBottomEltypeNoUnits, tTypeNoUnits}
    reduced_rate_prototype = rate_prototype.x[2]
    tab = FineRKN4ConstantCache(constvalue(uBottomEltypeNoUnits), constvalue(tTypeNoUnits))
    k1 = zero(rate_prototype)
    k2 = zero(reduced_rate_prototype)
    k3 = zero(reduced_rate_prototype)
    k4 = zero(reduced_rate_prototype)
    k5 = zero(reduced_rate_prototype)
    k = zero(rate_prototype)
    utilde = zero(u)
    atmp = similar(u, uEltypeNoUnits)
    recursivefill!(atmp, false)
    tmp = zero(u)
    FineRKN4Cache(u, uprev, k1, k2, k3, k4, k5, k, utilde, tmp, atmp, tab)
end

It is hard to spot where the function arguments end and the function body begins. Is there a way to let the formatter make this more obvious?

[ChrisRackauckas]

function alg_cache(alg::FineRKN4, u, rate_prototype, ::Type{uEltypeNoUnits},
    ::Type{uBottomEltypeNoUnits}, ::Type{tTypeNoUnits}, uprev, uprev2, f, t,
    dt, reltol, p, calck,
    ::Val{true}) where {uEltypeNoUnits, uBottomEltypeNoUnits, tTypeNoUnits}

    reduced_rate_prototype = rate_prototype.x[2]
    tab = FineRKN4ConstantCache(constvalue(uBottomEltypeNoUnits), constvalue(tTypeNoUnits))
    k1 = zero(rate_prototype)
    k2 = zero(reduced_rate_prototype)
    k3 = zero(reduced_rate_prototype)
    k4 = zero(reduced_rate_prototype)
    k5 = zero(reduced_rate_prototype)
    k = zero(rate_prototype)
    utilde = zero(u)
    atmp = similar(u, uEltypeNoUnits)
    recursivefill!(atmp, false)
    tmp = zero(u)
    FineRKN4Cache(u, uprev, k1, k2, k3, k4, k5, k, utilde, tmp, atmp, tab)
end

I'd just put a space in there. @YingboMa @issaacs ? I don't think we'd go back to the old rule of the argument indentation and want to look for something simple like this.

[Vaibhavdixit02]

The new line is definitely necessary

[isaacsas]

New line makes sense to me.

[YingboMa]

I prefer to revert https://github.com/domluna/JuliaFormatter.jl/pull/729

[sloede]

I prefer to revert #729

I can understand that 😬 If that's not an option, what about considering to at least add additional indentation, e.g., something like

function alg_cache(alg::FineRKN4, u, rate_prototype, ::Type{uEltypeNoUnits},
        ::Type{uBottomEltypeNoUnits}, ::Type{tTypeNoUnits}, uprev, uprev2, f, t,
        dt, reltol, p, calck,
        ::Val{true}) where {uEltypeNoUnits, uBottomEltypeNoUnits, tTypeNoUnits}
    reduced_rate_prototype = rate_prototype.x[2]
    tab = FineRKN4ConstantCache(constvalue(uBottomEltypeNoUnits), constvalue(tTypeNoUnits))
    k1 = zero(rate_prototype)
    k2 = zero(reduced_rate_prototype)
    k3 = zero(reduced_rate_prototype)
    k4 = zero(reduced_rate_prototype)
    k5 = zero(reduced_rate_prototype)
    k = zero(rate_prototype)
    utilde = zero(u)
    atmp = similar(u, uEltypeNoUnits)
    recursivefill!(atmp, false)
    tmp = zero(u)
    FineRKN4Cache(u, uprev, k1, k2, k3, k4, k5, k, utilde, tmp, atmp, tab)
end

[ChrisRackauckas]

@visr

[dawbarton]

Personally I prefer the double indent suggested by sloede. Doesn't result in cramped function arguments when you've got a long function name but provides visual distinction between arguments and body.

[adienes]

also an option to both double indent and newline when the signature spans multiple lines, which would be my first choice.

[visr]

Perhaps I put up #729 a bit too quickly and it should've been discussed more. But based on the Slack thread and examples like https://github.com/SciML/RuntimeGeneratedFunctions.jl/pull/64/commits/d41842e933b3c55bbed96552653a3c25a900d00c it seemed like most agreed that aligning on opening parentheses is not a good rule to always apply. So IMO we shouldn't revert.

The suggestion to indent extra from https://github.com/domluna/JuliaFormatter.jl/issues/741#issuecomment-1607442950 could alleviate the problem here, though personally I prefer what black does here, putting the ) on a new unindented line. That also leaves space for return type annotations or where statements. And with multi-line arguments I'd also put the first argument on a new line, like so:

function alg_cache(
    alg::FineRKN4, u, rate_prototype, ::Type{uEltypeNoUnits},
    ::Type{uBottomEltypeNoUnits}, ::Type{tTypeNoUnits}, uprev, uprev2, f, t,
    dt, reltol, p, calck,
    ::Val{true}
) where {uEltypeNoUnits, uBottomEltypeNoUnits, tTypeNoUnits}
    reduced_rate_prototype = rate_prototype.x[2]
    tab = FineRKN4ConstantCache(constvalue(uBottomEltypeNoUnits), constvalue(tTypeNoUnits))
    k1 = zero(rate_prototype)
    k2 = zero(reduced_rate_prototype)
    k3 = zero(reduced_rate_prototype)
    k4 = zero(reduced_rate_prototype)
    k5 = zero(reduced_rate_prototype)
    k = zero(rate_prototype)
    utilde = zero(u)
    atmp = similar(u, uEltypeNoUnits)
    recursivefill!(atmp, false)
    tmp = zero(u)
    FineRKN4Cache(u, uprev, k1, k2, k3, k4, k5, k, utilde, tmp, atmp, tab)
end

I mentioned this on the Slack thread and others mentioned that BlueStyle and others also do some of this, so it probably is more common than the extra indent.

[isaacsas]

That style has been discussed before I believe. Among other issues it breaks folding of functions and alignment of the first non-argument line in some editors (for example VSCode).

[visr]

@isaacsas if you can still find this discussion it'd be interesting to read. I tried in VS code and see:

1. Folding: there are indeed 2 folds, one for the arguments and one for the body. Not sure how that can be fixed, but for Python it is fixed.
2. If I press enter after function f( it does exactly what I want, arguments indented and closing ) unindented
3. To start the function body an extra tab is indeed needed since auto-indent doesn't do that for the first line. This is also not a problem for Python.

So they are valid point but IMO nothing too serious + fixable.

[isaacsas]

I don't recall where the discussion occurred unfortunately. Either a previous issue here or an older Slack.

But generally I wouldn't be in favor of any format change that results in non-indented lines between function and end. I think it is likely to lead to issues with editors, and personally find it harder to visually delineate functions. If we wanted to double indent function arguments and single indent the ) where that would be fine with me. (Though I still prefer the original suggestion to just add a newline over having the possibility of lines with just a dangling ).)

[thazhemadam]

I think the format suggested in https://github.com/domluna/JuliaFormatter.jl/issues/741#issuecomment-1607500407 looks great. There's a clear separation between the function header and the body, and the distinction between the function arguments and the type information is also immediately obvious.

[domluna]

a bit late to the discussion here but I'm happy to help/accept any PR you folks decide on.

[baggepinnen]

As @isaacsas has mentioned, it would be worth working out a solution to the editor problem. For example, the following behavior in vs-code is a nightmare

Screencast from 2023-06-27 08:10:52.webm

This appears not only when pressing enter after having written a function signature, it also appears when pasting code into the top of the function body, or moving code there from further down the function body.

[devmotion]

I guess most styles are just a matter of taste and I'm usually happy about the consistency introduced by them even if not every design choice is my personal favourite.

But I'm very glad that #729 was merged and would be a bit annoyed if it would be reverted - I think the previous nesting behaviour caused really unreasonable indentation. An example (which provoked this comment here) is https://github.com/SciML/MuladdMacro.jl/pull/42#discussion_r1243617530.

In many other projects I use BlueStyle so I think something like https://github.com/domluna/JuliaFormatter.jl/issues/741#issuecomment-1607500407 would be a good alternative. IMO if there are problems with editors, they should be fixed in these editors but not dictate the style choice (also because similar approaches are already used by other styles). I also tend to think that more lines are preferable to overly long lines.

[rmsrosa]

It always bothered me that vscode aligns the ending parenthesis to function. I always wished it would align to the body of the function, as follows:

function alg_cache(
        alg::FineRKN4, u, rate_prototype, ::Type{uEltypeNoUnits},
        ::Type{uBottomEltypeNoUnits}, ::Type{tTypeNoUnits}, uprev, uprev2, f, t,
        dt, reltol, p, calck,
        ::Val{true}
    ) where {uEltypeNoUnits, uBottomEltypeNoUnits, tTypeNoUnits}
    reduced_rate_prototype = rate_prototype.x[2]
    tab = FineRKN4ConstantCache(constvalue(uBottomEltypeNoUnits), constvalue(tTypeNoUnits))
    k1 = zero(rate_prototype)
    k2 = zero(reduced_rate_prototype)
    k3 = zero(reduced_rate_prototype)
    k4 = zero(reduced_rate_prototype)
    k5 = zero(reduced_rate_prototype)
    k = zero(rate_prototype)
    utilde = zero(u)
    atmp = similar(u, uEltypeNoUnits)
    recursivefill!(atmp, false)
    tmp = zero(u)
    FineRKN4Cache(u, uprev, k1, k2, k3, k4, k5, k, utilde, tmp, atmp, tab)
end

[isaacsas]

Given that this is stalled out, and could be discussed endlessly since it is a very subjective issue, perhaps we should just go with @sloede's suggestion of double indenting, maybe with a trailing extra newline as @ChrisRackauckas suggested? Double indenting seemed to have no real opposition, so perhaps it is a good compromise from the options in this discussion?

(I bring this up in the hope of getting some resolution, as I've been avoiding reformatting several libraries in anticipation of another change, but would like to stop seeing the resulting test failure indicators...)

[YingboMa]

Does the double indentation style cause problems in vscode? @baggepinnen

[isaacsas]

Code folding at least doesn't seem to have any issue with it.

[visr]

I tried to reproduce @baggepinnen's video here.

The same issues are there, but now things get indented one too far instead of too little. Also noticed another issue that BlueStyle/https://github.com/domluna/JuliaFormatter.jl/issues/741#issuecomment-1607500407 doesn't have: at the end I paste the formatted code and the whole function body loses one indent, which IMO is worse than having function argument code folding.

Though I agree with with https://github.com/domluna/JuliaFormatter.jl/issues/741#issuecomment-1609446875 not to let editor issues dictate style choice.

[isaacsas]

I find VSCode annoyingly loses indentation information quite frequently, whether it be with the current or previous style. I haven't tried to understand the conditions where it occurs though. Usually hitting undo restores the indentation without removing the pasted code so it can at least be fixed quickly.

Perhaps we can just go with Chris' original suggestion of adding a space then? That also didn't seem to have any strong arguments against it, and shouldn't change any pasting / folding behavior from the current style.

[ChrisRackauckas]

We should try to finalize this. Because:

https://github.com/SciML/SciMLBase.jl/pull/370/files/7a24ffd822aa2c7a13115d8f5d8e34e9eea37286..7cf85c43aced812d700445537c221758936e217f#diff-ac94ce23506dc1295d335638baf95d7fb3ec8739d4e76676f3e943bd1ef9ec68R214-R221

it looks like the default nesting is incorrect and is preferring to put everything on new lines after https://github.com/domluna/JuliaFormatter.jl/pull/729, instead of filling lines, which is something everyone agreed before was unsatisfactory. So now we are in a position of never wanting to run the formatter because it newlines everything, and so I would like to probably get that reverted ASAP and switch to whatever the final solution is here.

[ChrisRackauckas]

Okay this is getting crazy, we've effectively been ignoring the formatter since June because of #729. Do we just revert it or add the space? I think those are the two on the table. Let's make a decision this week. Vote yes for revert, <3 for space, and down if you like it as is. Rocket emoji for https://github.com/domluna/JuliaFormatter.jl/issues/741#issuecomment-1607442950 . Afterwards we keep the style to whatever is chosen and start using the formatter again.

But I think no matter what, can we get help @domluna figuring out what disabled the line filling and started putting every argument on a new line?

[adienes]

why is https://github.com/domluna/JuliaFormatter.jl/issues/741#issuecomment-1607500407 off the table? it sounds like that's a bug in VSCode extension not a fundamental problem with the format

[ChrisRackauckas]

Do you or someone else want to commit to fixing the VS Code extension by the end of next week? It's been a few months now and so I think we need to just get serious about a choice. If no one is stepping up to do it then we should just assume it's not going to be done. Going a whole year with having formatter failures is not an option, and telling everyone developing SciML to stop using VS Code is also not a reasonable option. Thus the 3 options there are the options that can reasonably be implemented with the available resources that I know of unless someone is working on the VS Code extension fix but hasn't shared WIP PRs or anything.

[isaacsas]

Moreover, other options that were discussed did not have arguments against them, unlike #741, and some had more apparent support in votes (https://github.com/domluna/JuliaFormatter.jl/issues/741#issuecomment-1607442950). Chris' suggestion of adding a space is a simple fix that can be made immediately, and was one that no one really voiced a strong opinion against.

[ChrisRackauckas]

Oh I missed https://github.com/domluna/JuliaFormatter.jl/issues/741#issuecomment-1607442950, that's a reasonable one too.

[ChrisRackauckas]

Edited the voting to add that one in.

[visr]

If https://github.com/domluna/JuliaFormatter.jl/issues/741#issuecomment-1607500407 is an option, why isn't https://github.com/domluna/JuliaFormatter.jl/issues/741#issuecomment-1607442950? In https://github.com/domluna/JuliaFormatter.jl/issues/741#issuecomment-1633147690 I showed that both options have the same VSCode issues, the former even a bit more.

you or someone else want to commit to fixing the VS Code extension by the end of next week

Just to understand, what is the most important issue here that needs to be fixed? The separate folding of args and body, or the manual indent needed when starting the body. The extra enter is just as much work as an extra tab, and the latter can go away if fixed in VSCode.