JuliaDiff / BlueStyle

A Julia style guide that lives in a blue world
Creative Commons Zero v1.0 Universal
489 stars 35 forks source link

Short-circuit logic used as control flow should only be used on a single line #72

Open nickrobinson251 opened 4 years ago

nickrobinson251 commented 4 years ago

This came up over at https://github.com/invenia/LibPQ.jl/pull/197#discussion_r492802769

e.g. for && and ||

# Yes
if last_log == curr
    debug(LOGGER, "Consuming input from connection $(jl_conn.conn). Stand by for landing.")
end

# No, over line limit:
last_log == curr && debug(LOGGER, "Consuming input from connection $(jl_conn.conn). Stand by for landing.")

# No, use an `if` conditional:
last_log == curr &&
    debug(LOGGER, "Consuming input from connection $(jl_conn.conn). Stand by for landing.")

(aside: we may want to use a different example in the guide, given #59 is an open question)

This is consistent with out current advice on ternary conditionals:

Ternary operators (?:) should generally only consume a single line

i.e.

# Yes:
foobar = if some_big_long_really_long_expr_here_long == 2
    barrrr_more_long
else
    bazzz_also_not_short
end

# No:
foobar = some_big_long_really_long_expr_here_long == 2 ? barrrr_more_long : bazzz_also_not_short

Unlike ternary conditionals ?:, chaning short-circuit logic as conditionals is fine e.g. this is okay

is_red(x) || is_blue(x) || is_yellow(x) && println("It's a primary colour!")
omus commented 4 years ago

is_red(x) || is_blue(x) || is_yellow(x) && println("It's a primary colour!")

Example is incorrect:

julia> true || false || false && println("it works!")
true
omus commented 4 years ago

A counter example which I am fine with:

last_log == curr && debug(LOGGER) do
    "Consuming input from connection $(jl_conn.conn). Stand by for landing.")
end

I think I'm okay with using short-circuit logic on multiple lines if the portion that spans multiple lines forms is contained within a block. e.g.:

condition && begin
    ...
end
nickrobinson251 commented 4 years ago

Example is incorrect

😆 Oooops

(is_red(x) || is_blue(x) || is_yellow(x)) && println("It's a primary colour!")

But i gues you get the point: we're fine with chaining &&/|| to the extent they fit on 1 line

nickrobinson251 commented 4 years ago

I think I'm okay with using short-circuit logic on multiple lines if the portion that spans multiple lines forms is contained within a block

I think i agree with this too 👍 (But would be okay recommending against it if other people felt strongly)

nickrobinson251 commented 4 years ago

Here's a specific case (from PkgTemplates) where we need to decide what style we want and what the formatter should do:


julia> str = """
           p.project && t.julia < v"1.2" && @warn string(
               "Tests: The project option is set to create a project (supported in Julia 1.2 and later) ",
               "but a Julia version older than 1.2 ((t.julia)) is supported by the template",
           )
       """;

julia> println(str)  # starting code
    p.project && t.julia < v"1.2" && @warn string(
        "Tests: The project option is set to create a project (supported in Julia 1.2 and later) ",
        "but a Julia version older than 1.2 ((t.julia)) is supported by the template",
    )

julia> println(format_text(str, BlueStyle()))  # formatted code with JuliaFormatter v0.9.7
p.project &&
    t.julia < v"1.2" &&
    @warn string(
        "Tests: The project option is set to create a project (supported in Julia 1.2 and later) ",
        "but a Julia version older than 1.2 ((t.julia)) is supported by the template",
    )
omus commented 4 years ago

I think this looks reasonable:

    p.project && t.julia < v"1.2" && @warn begin
        "Tests: The project option is set to create a project (supported in Julia 1.2 and later) " *
        "but a Julia version older than 1.2 ((t.julia)) is supported by the template"
    end