Skip to content

Parens around kwarg values is inconsistent and possibly could use an option #1016

@penelopeysm

Description

@penelopeysm

Consider:

s = """
(; a= >=(1))
(; a=2)
(; a=+2)
(; a=+x)
(; a=+x/2)
(; a=+(2))
"""

using JuliaFormatter: format_text as ft
ft(s; whitespace_in_kwargs=false) |> println

Looking at #360 and #361 the real underlying reason behind that PR was to make sure that the first of the above did not get converted into (; a=>=1), which would fail to parse as => is an operator. However, looking at the actual code in #361 it's very clear that the intent was to parenthesise anything on the RHS that began with an operator. This is overly cautious, because =+ is not a valid operator and thus things like (; a=+x) parse totally fine.

Now, recent versions of JuliaFormatter have led to changes in the underlying operator-detection mechanism. This is the outcome with several different versions of JuliaFormatter:

v1.0.62          v2.5.2           # 1011
(; a=(>=(1)))    (; a=(>=(1)))    (; a=(>=(1)))
(; a=2)          (; a=2)          (; a=2)
(; a=+2)         (; a=+2)         (; a=+2)
(; a=+x)         (; a=(+x))       (; a=(+x))
(; a=+x / 2)     (; a=+x/2)       (; a=(+x/2))
(; a=(+(2)))     (; a=(+(2)))     (; a=(+(2)))

In other words, we parenthesise more and more things. In my opinion, this is consistent with the intent of #361, and that's why #1011 specifically adds even more parens.

(Note that the +2 is an interesting case: +2 is parsed as an integer literal and not unary-plus applied to 2. Conversely, +x is parsed as unary-plus applied to x, which is why it gets parenthesised. One could argue that this is a bug.)

However, as said above, it's not really necessary to parenthesise these things. One could argue that it improves visual separation between the equals sign and the rhs. But one could also argue that it increases visual clutter. Given that there are arguments for and against it, I think the only fair thing to do is to have a configuration option parenthesize_kwargs_with_ops that controls whether parens are added. In particular:

  • this option should only matter when whitespace_in_kwargs=false because otherwise the space separation means that there's no need for parenthesisation;
  • this option should obviously still parenthesise >=(1) in order to avoid generating invalid code, but everything else should be unformatted as far as possible.

Thanks to the refactor done in #1011, though, this should be a lot easier to do. We simply need to pass a boolean into source_begins_with_op_needing_parens and change the final conditions in the return value accordingly.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions