JuliaFormatter.jl

JuliaFormatter.jl copied to clipboard

In using the formatter for Metalhead.jl (https://github.com/FluxML/Metalhead.jl/pull/163) with the sciml style, it seems to somehow want to keep the final two ends on the same line for testsets with a for. An example is given below:

@testset "VGG" begin @testset "VGG($sz, batchnorm=$bn)" for sz in [11, 13, 16, 19],
                                                            bn in [true, false]

    m = VGG(sz, batchnorm = bn)

    @test size(m(x_224)) == (1000, 1)
    if (VGG, sz, bn) in PRETRAINED_MODELS
        @test (VGG(sz, batchnorm = bn, pretrain = true); true)
    else
        @test_throws ArgumentError VGG(sz, batchnorm = bn, pretrain = true)
    end
    @test gradtest(m, x_224)
    GC.safepoint()
    GC.gc()
end end

it looks like the @testset for is being lifted up for some reason. Looking at the PR it wasn't like that in the original source

@YingboMa

It's because of https://github.com/SciML/SciMLStyle#tests-and-continuous-integration . It's applied generally.

Is there a way to toggle that particular option in the configuration file?

Bump on this. Is https://github.com/domluna/JuliaFormatter.jl/blob/master/src/styles/sciml/pretty.jl#L93-L120 responsible for unconditionally applying this formatting? If so, having an option to disable it and use the default p_begin behaviour would be swell.

I think it might be. I have to take another look at this, macros are tricky, maybe #669 can be dealt with at the same time

so it's indeed because of https://github.com/domluna/JuliaFormatter.jl/blob/master/src/styles/sciml/pretty.jl#L93-L120

Is this intentional then? How should we proceed @YingboMa @ToucheSir

toggling this isn't exactly what options are designed for.

Can we adjust this so that if it's a macrocall then it is intended and placed on a new line?

so changing line 103 to

if length(stmts_idxs) == 1 && !(cst[2].head === :macrocall && !is_closer(cst[2][end]))

ok https://github.com/domluna/JuliaFormatter.jl/pull/673 this seems sufficient no?

If I understand the rule correctly, this is something that depends on line length as well? I don't understand the test in #673 well enough to know whether that's the globally desirable behaviour, but if the example in the OP is fixed then that looks good to me.

        stmts_idxs = 2:length(cst)-1
        # Don't nest into multiple lines when there's only one statement
        # and it's not a macroblock.
        if length(stmts_idxs) == 1 && !(cst[2].head === :macrocall && !is_closer(cst[2][end]))
            add_node!(t, Whitespace(1), s)
            add_node!(t, pretty(style, cst[2], s), s, join_lines = true)
            add_node!(t, Whitespace(1), s)
            add_node!(t, pretty(style, cst[end], s), s, join_lines = true)
        else

this block forces line length to explicitly not be considered. The wording in the doc Yingbo linked and the comment he wrote initially in the above code implies it's more so about having a singular expression/statement vs. length

@YingboMa does this look reasonable?

the original example becomes

julia> format_text(s, SciMLStyle()) |> print
@testset "VGG" begin
    @testset "VGG($sz, batchnorm=$bn)" for sz in [11, 13, 16, 19],
                                           bn in [true, false]

        m = VGG(sz, batchnorm = bn)

        @test size(m(x_224)) == (1000, 1)
        if (VGG, sz, bn) in PRETRAINED_MODELS
            @test (VGG(sz, batchnorm = bn, pretrain = true); true)
        else
            @test_throws ArgumentError VGG(sz, batchnorm = bn, pretrain = true)
        end
        @test gradtest(m, x_224)
        GC.safepoint()
        GC.gc()
    end
end

is this fine to merge SciML people?

That looks fine.