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

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

Open
nickrobinson251 opened this issue Sep 24, 2020 · 6 comments

Comments

@nickrobinson251
Copy link
Contributor

nickrobinson251 commented Sep 24, 2020

This came up over at JuliaDatabases/LibPQ.jl#197 (comment)

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
Copy link
Contributor

omus commented Sep 25, 2020

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
Copy link
Contributor

omus commented Sep 25, 2020

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
Copy link
Contributor Author

nickrobinson251 commented Sep 25, 2020

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
Copy link
Contributor Author

nickrobinson251 commented Sep 25, 2020

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
Copy link
Contributor Author

nickrobinson251 commented Sep 29, 2020

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
Copy link
Contributor

omus commented Sep 30, 2020

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

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

No branches or pull requests

2 participants