Add spaces around = for kwargs

[Chandu-4444]

I tried to make deprecations.jl and utils.jl adhere to code style i.e to have spaces around = for kwargs as mentioned here #1765.

[ToucheSir]

I'm generally ok with this, but we ought to get a consensus among the committers because it will have a huge impact on anything that requires looking back through diffs (most notably git blame). Hence it's best to first have an agreement about the scope of the change. Do we format all files at once or piecemeal? Do we hit all style guide aspects in one go or in multiple rounds?

But most importantly, what is the actual style convention in its totality? This includes stuff that was not yet covered in #1765 like line breaking and alignment indentation. Also, in case you haven't done so already, I would highly recommend creating a JuliaFormatter config first and only then working through files. This will be required eventually anyways as part of the repo (if not as part of the first formatting-related PR), so not doing so now runs the risk of having to redo a bunch of work later.

[DhairyaLGandhi]

I think this is fine to go with as it is. This is surely not hitting all the files, but it makes a decent attempt at getting things in order. We prefer linux file endings though. That helps not have the pesky Red Cross at the end of files on GitHub.

[ToucheSir]

In that case, I'd like to see a JuliaFormatter config with all the points mentioned so far (including line endings) and the application of that config to these 2 files. If you're using VSCode, the Julia extension now bundles JuliaFormatter by default, so call to autoformat should be all it takes :)

[Chandu-4444]

In that case, I'd like to see a JuliaFormatter config with all the points mentioned so far (including line endings) and the application of that config to these 2 files. If you're using VSCode, the Julia extension now bundles JuliaFormatter by default, so call to autoformat should be all it takes :)

Umm.. It seems like I need to specify a reasonable value for margin apart from line endings and newline style. The lines are getting wrapped to a fixed length (But currently there seemed to be no fixed length for the files before they get wrapped) should this also be standardised?

Currently, contents of .JuliaFormatter.toml has the following:

indent = 4
whitespace_in_kwargs = true
normalize_line_endings = "UNIX"

[ToucheSir]

Could you keep this PR updated with the config file and formatted source files so that we can see behaviour like

The lines are getting wrapped to a fixed length (But currently there seemed to be no fixed length for the files before they get wrapped)

firsthand?

Back to the style guide, wrapping to a fixed length is expected and should be happening. The only reason we don't do so already is because there hasn't ever been a tool enforcing it (hence this PR). As for what margin to use, unless anyone has strong opinions to the contrary I'd say just stick to the default of 92.

[ToucheSir]

Just tested locally and I think I see your concern with wrapping. Using the above config, this:

@deprecate InstanceNorm(λ, β, γ, μ, σ², ϵ, momentum, active=nothing) InstanceNorm(λ, β, γ, μ, σ², ϵ, momentum, true, true, active, length(β))
@deprecate BatchNorm(λ, β, γ, μ, σ², ϵ, momentum, active=nothing) BatchNorm(λ, β, γ, μ, σ², ϵ, momentum, true, true, active, length(β))
@deprecate GroupNorm(G, λ, β, γ, μ, σ², ϵ, momentum, active=nothing) GroupNorm(G, λ, β, γ, μ, σ², ϵ, momentum, true, true, active, length(β))

turns into this:

@deprecate InstanceNorm(λ, β, γ, μ, σ², ϵ, momentum, active = nothing) InstanceNorm(
    λ,
    β,
    γ,
    μ,
    σ²,
    ϵ,
    momentum,
    true,
    true,
    active,
    length(β),
)
@deprecate BatchNorm(λ, β, γ, μ, σ², ϵ, momentum, active = nothing) BatchNorm(
    λ,
    β,
    γ,
    μ,
    σ²,
    ϵ,
    momentum,
    true,
    true,
    active,
    length(β),
)
@deprecate GroupNorm(G, λ, β, γ, μ, σ², ϵ, momentum, active = nothing) GroupNorm(
    G,
    λ,
    β,
    γ,
    μ,
    σ²,
    ϵ,
    momentum,
    true,
    true,
    active,
    length(β),
)

For reference, here's style="yas":

@deprecate InstanceNorm(λ, β, γ, μ, σ², ϵ, momentum; active = nothing) InstanceNorm(λ, β, γ,
                                                                                    μ, σ²,
                                                                                    ϵ,
                                                                                    momentum,
                                                                                    true,
                                                                                    true,
                                                                                    active,
                                                                                    length(β))
@deprecate BatchNorm(λ, β, γ, μ, σ², ϵ, momentum; active = nothing) BatchNorm(λ, β, γ, μ,
                                                                              σ², ϵ,
                                                                              momentum,
                                                                              true, true,
                                                                              active,
                                                                              length(β))
@deprecate GroupNorm(G, λ, β, γ, μ, σ², ϵ, momentum; active = nothing) GroupNorm(G, λ, β, γ,
                                                                                 μ, σ², ϵ,
                                                                                 momentum,
                                                                                 true, true,
                                                                                 active,
                                                                                 length(β))

And style="blue"

@deprecate InstanceNorm(λ, β, γ, μ, σ², ϵ, momentum; active = nothing) InstanceNorm(
    λ, β, γ, μ, σ², ϵ, momentum, true, true, active, length(β)
)
@deprecate BatchNorm(λ, β, γ, μ, σ², ϵ, momentum; active = nothing) BatchNorm(
    λ, β, γ, μ, σ², ϵ, momentum, true, true, active, length(β)
)
@deprecate GroupNorm(G, λ, β, γ, μ, σ², ϵ, momentum; active = nothing) GroupNorm(
    G, λ, β, γ, μ, σ², ϵ, momentum, true, true, active, length(β)
)

BlueStyle seems the most conservative of the bunch, so perhaps it's worth basing the config off that instead of the default (AFAICT there is no way to configure this wrapping indent behaviour without changing the style).

[Chandu-4444]

Please look at these formatted files.
style = 'blue' seems like a good option, but there are a few places where the previous format looked good rather than this.

["crim","zn","indus","chas","nox","rm","age","dis","rad","tax","ptratio","b","lstat"]

Got formatted to:

    return [
        "crim",
        "zn",
        "indus",
        "chas",
        "nox",
        "rm",
        "age",
        "dis",
        "rad",
        "tax",
        "ptratio",
        "b",
        "lstat",
    ]
end

The are few more places where the earlier format looked efficient.

Also, I formatted all files in src/ so that how the contents of all files will look like obeying the specified style in the format file.

[ToucheSir]

That entire folder is due to be removed soon with v0.13, so I wouldn't worry about it too much. The other stuff seems mostly wrapping related, so we'll want to wait for at least a couple other folks to chime in. I'll also walk through the changes over the next few days and put in suggestions for places where we might be able to appease the formatter.

[Chandu-4444]

That entire folder is due to be removed soon with v0.13, so I wouldn't worry about it too much. The other stuff seems mostly wrapping related, so we'll want to wait for at least a couple other folks to chime in. I'll also walk through the changes over the next few days and put in suggestions for places where we might be able to appease the formatter.

Yeah sure 👍

[DhairyaLGandhi]

I haven't gone through all the changes but it seems most of these are not really necessary?

[DhairyaLGandhi]

We don't need to change this

[ToucheSir]

I believe this is because the original line(s) were too long. Grouping into multiple export statements per https://github.com/invenia/BlueStyle#function-exports would get as back to if not under the original line count.

[darsnack]

Personally, I think I'd like to see this PR with the YAS style (seems the least pedantic to me) and the use of # !format: off used for src/Flux.jl. That file requires human intervention for what's best. Could do the same for deprecations if it is an issue. I don't really think anyone cares about the readability of deprecations.jl.

[ToucheSir]

I suggested BlueStyle because the default line break/wrapping seemed closer to what was currently in-repo, but if it's possible to dissuade the YAS style from doing crazy multi-line splits then that would be fine too.

[darsnack]

Is there not a style guide for the following:

f(x, y, z; p, q, r) # if it all fits
f(x, y, z;
  p, q, r) # first stage
f(x, y, z;
  p,
  q,
  r) # second stage
f(x,
  y,
  z;
  p,
  q,
  r) # last stage

# Long array literals
x = [a,
     b,
     c,
     d,
     e,
     f,
     g]

These are at least the conventions that would selfishly please me, but I'm happy to go with whatever style everyone likes best. I'd just rather have a formatter bot than needing review with formatting comments.

[ToucheSir]

This is what I see with the YAS preset in JuliaFormatter:

f(long_arg_name_x, long_arg_name_y, long_arg_name_z; kwarg_p, kwarg_q, kwarg_r)

f(long_arg_name_x, long_arg_name_y, long_arg_name_z; long_kwarg_name_p, long_kwarg_name_q,
  long_kwarg_name_r)

f(long_arg_name_x, long_arg_name_y, long_arg_name_z; long_long_long_kwarg_name_p,
  long_long_long_kwarg_name_q, long_long_long_kwarg_name_r)

f(arg_name_that_is_almost_as_large_as_the_max_line_length_1,
  arg_name_that_is_almost_as_large_as_the_max_line_length_2,
  arg_name_that_is_almost_as_large_as_the_max_line_length_3,
  arg_name_that_is_almost_as_large_as_the_max_line_length_4,
  arg_name_that_is_almost_as_large_as_the_max_line_length_5)

x = ["the quick brown fox jumped over the lazy dog",
     "the quick brown fox jumped over the lazy dog",
     "the quick brown fox jumped over the lazy dog",
     "the quick brown fox jumped over the lazy dog",
     "the quick brown fox jumped over the lazy dog",
     "the quick brown fox jumped over the lazy dog",
     "the quick brown fox jumped over the lazy dog"]

c.f. the default "JuliaFormatter" style (BlueStyle is the same here):

f(long_arg_name_x, long_arg_name_y, long_arg_name_z; kwarg_p, kwarg_q, kwarg_r)

f(
    long_arg_name_x,
    long_arg_name_y,
    long_arg_name_z;
    long_kwarg_name_p,
    long_kwarg_name_q,
    long_kwarg_name_r,
)

f(
    long_arg_name_x,
    long_arg_name_y,
    long_arg_name_z;
    long_long_long_kwarg_name_p,
    long_long_long_kwarg_name_q,
    long_long_long_kwarg_name_r,
)

f(
    arg_name_that_is_almost_as_large_as_the_max_line_length_1,
    arg_name_that_is_almost_as_large_as_the_max_line_length_2,
    arg_name_that_is_almost_as_large_as_the_max_line_length_3,
    arg_name_that_is_almost_as_large_as_the_max_line_length_4,
    arg_name_that_is_almost_as_large_as_the_max_line_length_5,
)

x = [
    "the quick brown fox jumped over the lazy dog",
    "the quick brown fox jumped over the lazy dog",
    "the quick brown fox jumped over the lazy dog",
    "the quick brown fox jumped over the lazy dog",
    "the quick brown fox jumped over the lazy dog",
    "the quick brown fox jumped over the lazy dog",
    "the quick brown fox jumped over the lazy dog",
]

One thing we could try is taking a smaller repo (maybe Metalhead since you've got the deepest knowledge of the internals out of everyone here?) and creating 3 PRs, one for each JuliaFormatter base style. Then there's a side-by-side comparison of each style on a non-trivial codebase, and we can tweak additional params to converge on something good.