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

Apply Runic.jl formatter #613

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

efaulhaber
Copy link
Member

The Trixi.jl crew wanted to check out this new formatter. Here's what it would do to TrixiParticles.jl.

@svchb
Copy link
Collaborator

svchb commented Sep 23, 2024

I don't like it...
@efaulhaber and you?

@efaulhaber
Copy link
Member Author

I think it's pretty good.
What we currently have, looks like this for functions:

julia> println(format_text(str1, SciMLStyle(), yas_style_nesting = true))
function my_large_function(argument1, argument2,
                           argument3, argument4,
                           argument5, x, y, z)
    foo(x) + goo(y)
end

That's IMO nicely readable, but it's a bit annoying to format, as vscode doesn't automatically align the second line like for example PyCharm does (please tell me if there is an option or extension that can do that).

The problem is that this style seems to break sometimes, as it's not very commonly used and basically our own Frankenstein's monster. I added the yas_style_nesting option because we didn't agree with the new SciML style. SciML could decide at any time to change their style to something bad again, and this would again require a PR from us to keep our style.
I don't really like the other styles in JuliaFormatter. For example, regular SciML style looks like this:

julia> println(format_text(str1, SciMLStyle()))
function my_large_function(argument1, argument2,
        argument3, argument4,
        argument5, x, y, z)
    foo(x) + goo(y)
end

I don't like that the arguments in the first line are not close to the arguments in the following lines.

With Runic, we get this:

julia> println(Runic.format_string(str1))
function my_large_function(
        argument1, argument2,
        argument3, argument4,
        argument5, x, y, z
    )
    foo(x) + goo(y)
end

Which looks very consistent to me. You find all arguments at the very first look because they're all together and always with the same indent.

One thing I don't like is that aligning struct fields doesn't work with Runic.
But that's okay for me. Quoting the README:

Similarly to gofmt, Runic have no configuration. The formatting rules are set in stone (although not yet complete). This approach is something that is appreciated by most Go programmers, see for example the following quote:

Gofmt's style is no one's favorite, yet gofmt is everyone's favorite.

Just like PEP 8 in Python. I don't think about what I would do differently. I just appreciate that most Python code looks the same.

So, bottom line: I like that Runic wants to force one single style in this chaos of different styles. The Julia community should've done that from the start. Of course, this only works when we get most people to start using Runic.

The is however one (IMO big) caveat: Runic doesn't automatically break lines. So our format check doesn't catch overlong lines of code. We also can't just add a hard limit, as some lines just might not be able to be formatted nicely to fit the limit. Mostly URLs in comments. The current format check relies on JuliaFormatter automatically breaking the line. It basically fails if JuliaFormatter says "there is a nice way to make this fit the margin".
Here is an issue for that: fredrikekre/Runic.jl#61

@LasNikas
Copy link
Collaborator

Honestly, on first glance, I really didn't like it. However, your detailed argumentation make me think about it.
Is this something you want to have in near future? Probably it might be best to wait how Runic.jl will evolve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants