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

Implement BlueStyle #283

Open
24 of 36 tasks
nickrobinson251 opened this issue Sep 5, 2020 · 37 comments · Fixed by #308
Open
24 of 36 tasks

Implement BlueStyle #283

nickrobinson251 opened this issue Sep 5, 2020 · 37 comments · Fixed by #308

Comments

@nickrobinson251
Copy link
Contributor

nickrobinson251 commented Sep 5, 2020

This issue is to organise and track efforts to implement BlueStyle as a Custom Style. Similar to how YASGuide is now available in JuliaFormatter via YASStyle. (see issue #198 and PRs #214, #217).

I think the first step is to come up with a list of cases where BlueStyle recommends a style change that is currently not available via DefaultStyle or YASStyle...

TO DOs (see comment below for details/examples):

Still to look into:

  • Formatting docstrings

Could maybe also look into, but leaving these for now:

  • Testsets formatting
  • Test Comparisons

Going through this i realise that BlueStyle has a lot of things that cannot be automated, and that is fine. I think the intended use case for the formatter is an environment where collaborators know what style is intended and attempt to follow it, but don't have to sweat the details and don't have to rely on code review to fix whitespace style issues. I think this also means that its much more important that the formatter doesn't make "good" style into "bad" style, than it is to make all "bad" cases "good" (and all cases wouldn't be realistic anyway).

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Sep 5, 2020

Module imports

  • Module imports should occur at the top of the file or right after a module declaration

  • one import per line
# Yes:
using A
using B

# No:
using A, B

  • order imports alphabetically
# Yes:
using A
using B

# No:
using B
using A

  • Prefer using over import(this is partially supported already)
# Yes:
using Example

Example.hello(x::Monster) = "Aargh! It's a Monster!"
Base.isreal(x::Ghost) = false

# No:
import Base: isreal
import Example: hello

hello(x::Monster) = "Aargh! It's a Monster!"
isreal(x::Ghost) = false

By partially supported I mean:

julia> str = """
       import Example: hello
       import Foo
       """;

julia> print(format_text(str, import_to_using=true))
import Example: hello  # <-- for BlueStyle should be `using Example: hello`, even though this can break code (see below)
using Foo: Foo
  • I don't think we do the full code re-write for this example, but we can do import Example: hello - > using Example: hello.
  • This one's still a bit complicated in the sense that changing import Example: hello to using Example: hello can break code...
    • with using Example: hello, the code hello(x::Monster) = "Aargh!" no longer successfully adds a method;
    • andExample.hello also wouldn't work, unless the code was changed to using Example or using Example: Example, hello.

But I think that's fine for following BlueStyle, since the purpose of this rule is "to ensure that extension of a function is always explicit and on purpose", so we'd want CI to fail in this case.

(I also think this is fine because I see JuliaFormatter as a great tool for "fixing up" code to follow the style guide you intended -- but realistically the code has to be written with some mind to that style in the first place -- not to entirely rewrite any possible given code (however far it deviates from the style guide) to have then be exactly as if rewritten by someone following the guide. If that makes sense....)

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Sep 5, 2020

Method definitions

  • Only use short-form function definitions when they fit on a single line (already supported)
julia> str = """
       foo(x::Int64) = abs(x) + 3

       foobar(array_data::AbstractArray{T}, item::T) where {T<:Int64} = T[
           abs(x) * abs(item) + 3 for x in array_data
       ]

       foobar(
           array_data::AbstractArray{T},
           item::T,
       ) where {T<:Int64} = T[abs(x) * abs(item) + 3 for x in array_data]
       """;

julia> print(format_text(str; short_to_long_function_def=true))
foo(x::Int64) = abs(x) + 3

function foobar(array_data::AbstractArray{T}, item::T) where {T<:Int64}
    T[abs(x) * abs(item) + 3 for x in array_data]
end

function foobar(array_data::AbstractArray{T}, item::T) where {T<:Int64}
    T[abs(x) * abs(item) + 3 for x in array_data]
end

  • When using long-form functions always use the return keyword (partially supported?)
julia> str = """
       function Foo(x, y)
           new(x, y)
       end
       """;

julia> print(format_text(str; always_use_return=true))
function Foo(x, y)
    return new(x, y)
end

  • Use return nothing instead of bare return

  • Functions definitions with parameter lines which exceed 92 characters should separate each parameter by a newline and indent by one-level (already supported)

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Sep 5, 2020

Keyword arguments

  • No whitespace in keyword arguments (already supported)
julia> str = """
       xy = foo(x; y = 3)
       ab = foo(; a= 1, b= 2)
       """;

julia> print(format_text(str; whitespace_in_kwargs=false))
xy = foo(x; y=3)
ab = foo(; a=1, b=2)

  • Always separate your keyword arguments from your positional arguments with a semicolon (I think YASStyle also prefers this Full YASGuide implementation #198)
    • xy = foo(x, y=3) should become xy = foo(x; y=3)
    • ab = foo(a=1, b=2) could become ab = foo(; a=1, b=2) (or i think we'd be fine if it was left as ab = foo(a=1, b=2)?)
    • ab = foo(; a=1, b=2) should be left as ab = foo(; a=1, b=2)

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Sep 5, 2020

WhiteSpace

  • Avoid extraneous whitespace immediately inside parentheses, square brackets or braces
julia> str = """
       spam( Ham{ T }( ), [ eggs ] )
       """;

julia> print(format_text(str))
spam(Ham{T}(), [eggs])

  • Avoid extraneous whitespace immediately before a comma or semicolon (already supported I think?)
julia> str = """
       @show(x, y); x , y = y ;
       """;

julia> print(format_text(str))
@show(x, y);
x, y = y;

  • Avoid whitespace around : in ranges. Use brackets to clarify expressions on either side. (already supported -- and i'm very impressed by this one)
julia> str = """
       ham[1: 9]
       ham[9 : -3: 1]
       ham[lower : upper - 1]
       ham[lower + offset:upper + offset]
       """;

julia> print(format_text(str; whitespace_ops_in_indices=true))
ham[1:9]
ham[9:-3:1]
ham[lower:(upper - 1)]
ham[(lower + offset):(upper + offset)]

  • Avoid using more than one space around an assignment (or other) operator to align it with another
julia> str = """
       x             =  1
       y             =  2
       long_variable = 30
       """;

julia> print(format_text(str))
x = 1
y = 2
long_variable = 30

  • Surround most binary operators with a single space on either side, excluding , :,^ and //) (Almost supported already)
julia> str = """
       i=j+1
       submitted +=1
       x^2<y
       x->x>1
       3!=2
       1 : 2
       1//2
       """;

julia> print(format_text(str))
i = j + 1
submitted += 1
x^2 < y  # <-- Nice!
x -> x > 1
3 != 2
1:2
1 // 2  # <-- BlueStyle would prefer 1//2 here, i think

If we want // to be treated like ^ i think (maybe?) we'd want to include Tokens.FWDFWD_SLASH like Tokens.CIRCUMFLEX_ACCENT is in p_binaryopcall

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Sep 5, 2020

Whitespace (continued)

  • Avoid using whitespace between unary operands and the expression. (Already supported)
    • Here's what BlueStyle says:
       # Yes:
       -1
       [1 0 -1]
      
       # No:
       - 1
       [1 0 - 1]  # Note: evaluates to `[1 -1]`
      
    • Here's what JuliaFormatter does:
      julia> str = """
         - 1
         [- 1 0 - 1]
         [-1 0  -1]
         """;
      
      julia> print(format_text(str))
      -1
      [-1 2 - 1]  
      [-1 2 -1]
      • I think maybe both are right here. i.e. if following BlueStyle, you should never write [- 1 2 - 1] (instead write [-1 1]). But if you do write this, you cannot expect the Formatter to correct you, because the Formatter doesn't do arithmetic.

  • Avoid extraneous empty lines. (Already supported)
julia> str = """
       domath(x::Number) = x + 5
       domath(x::Int) = x + 10



       function domath(x::String)
           return "A string is a one-dimensional extended object postulated in string theory."
       end


       dophilosophy() = "Why?"
       """;

julia> print(format_text(str; remove_extra_newlines=true))
domath(x::Number) = x + 5
domath(x::Int) = x + 10

function domath(x::String)
    return "A string is a one-dimensional extended object postulated in string theory."
end

dophilosophy() = "Why?"

  • Function calls which cannot fit on a single line within the line limit should be broken up such that the lines containing the opening and closing brackets are indented to the same level while the parameters of the function are indented one level further. In most cases the arguments and/or keywords should each be placed on separate lines. (Already supported 💥)
julia> str = """
       f(
           a,
           b,
       )
       constraint = conic_form!(SOCElemConstraint(temp2 + temp3,
                                                  temp2 - temp3, 2 * temp1),
                                unique_conic_forms)
       """;

julia> print(format_text(str))
f(a, b)
constraint = conic_form!(
    SOCElemConstraint(temp2 + temp3, temp2 - temp3, 2 * temp1),
    unique_conic_forms,
)
julia> str = """
       f(
           a,
           b,
       )
       constraint = conic_form!(SOCElemConstraint(temp2 + temp3,
                                                  temp2 - temp3, 2 * temp1, "other reallllllly long arg"),
                                unique_conic_forms)
       """;

julia> print(format_text(str))
f(a, b)
constraint = conic_form!(
    SOCElemConstraint(
        temp2 + temp3,
        temp2 - temp3,
        2 * temp1,
        "other reallllllly long arg",
    ),
    unique_conic_forms,
)

(^ I'm so impressed by this)

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Sep 5, 2020

Whitespace (continued again)


  • Assignments using expanded notation for arrays or tuples, or function calls should have the first open bracket on the same line assignment operator and the closing bracket should match the indentation level of the assignment. Alternatively you can perform assignments on a single line when they are short. (Almost supported)

    • Works for arrays or tuples
    julia> str = """
           arr =
           [
               1,
               2,
           3,
       ]
       arr =
       [
           1 2 3
       ]
       arr = [
           1
           2
           3
           ]
       tup = (
           1, 2, 3
       )
       """;
    
    julia> print(format_text(str))
    arr = [1, 2, 3]
    arr = [1 2 3]
    arr = [
        1
        2
        3
    ]
    tup = (1, 2, 3)
    • Doesn't do the right thing for functions?
    julia> str = """
           some_big_long_variable_that_takes_up_most_of_the_line_but_still_leaves_room_for_f = f(
               "argument doesn't fit on that first line", 2
           )
           """;  # <-- BlueStyle thinks this is preferable, with `= f(` on that first line; don't change anything
    
    julia> print(format_text(str))
    some_big_long_variable_that_takes_up_most_of_the_line_but_still_leaves_room_for_f =
        f("argument doesn't fit on that first line", 2)
    julia> str = """
           quite_long_variable_that_takes_up_space = f(
               "argument doesn't fit on that first line because now it's longer", 2
           )
           """;  # <-- BlueStyle thinks this is preferable, with `= f(` on that first line; don't change anything
    
    julia> print(format_text(str))
    quite_long_variable_that_takes_up_space =
        f("argument doesn't fit on that first line because now it's longer", 2)

@domluna
Copy link
Owner

domluna commented Sep 5, 2020

julia> str = """
quite_long_variable_that_takes_up_space = f(
"argument doesn't fit on that first line because now it's longer", 2
)
"""; # <-- BlueStyle thinks this is preferable, with = f( on that first line; don't change anything

julia> print(format_text(str))
quite_long_variable_that_takes_up_space =
f("argument doesn't fit on that first line because now it's longer", 2)

First it places the RHS on the next line and if that's not gonna it lifts the function call f( to back up and the arguments each take up 1 line.

quite_long_variable_that_takes_up_space = f(
    "argument doesn't fit on that first line because now it's longer",
    2,
)

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Sep 5, 2020

yah, either of these would be okay:

quite_long_variable_that_takes_up_space = f(
    "argument doesn't fit on that first line because now it's longer", 2
)
quite_long_variable_that_takes_up_space = f(
    "argument doesn't fit on that first line because now it's longer",
    2,
)

(maybe the second one is preferable, but crucially: the = f( being on the first line, then next lines indented once, and closing bracket on own line)

Is there a way to get that result already (I couldn't find one), or is it something we'd need to add for BlueStyle?

Running format_text (with no other settings) on

quite_long_variable_that_takes_up_space = f(
    "argument doesn't fit on that first line because now it's longer",
    2,
)

currently alters it and returns

quite_long_variable_that_takes_up_space =
    f("argument doesn't fit on that first line because now it's longer", 2)

@domluna
Copy link
Owner

domluna commented Sep 5, 2020

It would probably be BlueStyle specific since I don't think it would be suitable as an option (seems it would be confusing). Doing it the way you mentioned is actually simpler from the formatter's point of view so that's a silver lining.

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Sep 6, 2020

Whiespace (continued again, again)

  • Always include the trailing comma when working with expanded arrays, tuples or functions notation (already supported)
julia> str = """
       arr1 = [
           1,
           2,
       ]
       arr2 = [
           "realllllllllllllllllllllllllllllly long one",
           "realllllllllllllllllllllllllllllly long two"
       ]
       tup = (
           1,
       )
       """;

julia> print(format_text(str))
arr1 = [1, 2]
arr2 = [
    "realllllllllllllllllllllllllllllly long one",
    "realllllllllllllllllllllllllllllly long two",
]
tup = (1,)

  • Triple-quotes... idented and, if being assigned, starting on the same line as the =
# Yes:
str2 = """
         hello
    world!
    """

# No:
str2 = """
    hello
world!
"""

str2 = 
    """
         hello
    world!
    """

  • Use blank-lines to separate different multi-line blocks.
    • TODO: I think this needs translating. Does it just mean "have a blank line after end"?
    • If so, then this later rule is just a special case of this one: "Use line breaks between control flow statements and returns." (assuming "control flow" here means only while.. end and if... end statements, which it seems to be, based on the example given).
    • I'd rather go with a sipler rule, e.g. "Add a blank line between multi-line blocks". But i'll move that discussion to Simplify some current guidance to "have a blank line between multi-line blocks"? JuliaDiff/BlueStyle#61

  • After a function definition, and before an end statement do not include a blank line.
    • This is not currently support, as far as i can tell
julia> str = """
       function foo(bar::Int64, baz::Int64)

           return bar + baz
       end

       function foo(bar::In64, baz::Int64)
           return bar + baz

       end
       """;

julia> print(format_text(str; remove_extra_newlines=true))
function foo(bar::Int64, baz::Int64)

    return bar + baz
end

function foo(bar::In64, baz::Int64)
    return bar + baz

end

should both become:

function foo(bar::In64, baz::Int64)
    return bar + baz
end

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Sep 6, 2020

NamedTuples

  • The = character in NamedTuples should be spaced as in keyword arguments. No space should be put between the name and its value.
julia> str = """
       xy = (x = 1, y = 2)
       """;

julia> print(format_text(str, whitespace_in_kwargs=false))
xy = (x=1, y=2)

julia> str = """
       xy = (; x = 1, y = 2)
       """;

julia> print(format_text(str, whitespace_in_kwargs=false))
xy = (; x=1, y=2)  # <-- should be `xy = (x=1, y=2)`, under current guidance, apparently

  • The empty NamedTuple should be written NamedTuple() not (;)
    julia> str = """
           nt = (;)
           """;
    
    julia> print(format_text(str))
    nt = ()
    • Desired behaviour when using BlueStyle is:
    julia> str = """
           nt = (;)
           """;
    
    julia> print(format_text(str))
    nt = NamedTuple()

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Sep 6, 2020

Numbers

  • Floating-point numbers should always include a leading and/or trailing zero (Almost already supported)

  • For leading 0

julia> str = ".1";

julia> print(format_text(str))
0.1
  • For trailing 0
julia> str = "2.";

julia> print(format_text(str))
2.0
julia> str = "3.f0";

julia> print(format_text(str))
3.f0

Desired output is 3.0f0 like

julia> 3.f0
3.0f0

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Sep 6, 2020

Ternary Operator

  • Ternary operators ?: should generally only consume a single line
# Yes:
foobar = foo == 2 ? bar : baz

# No:
foobar = foo == 2 ?
    bar :
    baz

Currently supported for the case where it can fit on one line

julia> str = """
       foobar = foo == 2 ?
           bar :
           baz
       """;

julia> print(format_text(str))
foobar = foo == 2 ? bar : baz

But i think BlueStyle prefers if... else... if the whole thing cannot fit on one line e.g.

# Yes:
foobar = if some_big_long_thing * 10_000 == 2
    bar
else
    another_big_long_thing * 10^300 / this_things_here
end

# No:
foobar = some_big_long_thing * 10_000 == 2 ?
    bar :
    another_big_long_thing * 10^300 / this_things_here

whereas JuliaFormatter goes to:

julia> str = """
       foobar = some_big_long_thing * 10_000 == 2 ?
           bar :
           another_big_long_thing * 10^300 / this_things_here
       """;

julia> print(format_text(str))
foobar = some_big_long_thing * 10_000 == 2 ? bar :
    another_big_long_thing * 10^300 / this_things_here

  • Do not chain multiple ternary operators. If chaining many conditions, consider using an if-elseif-else conditional
# Yes
foobar = if foo == 2
    bar
elseif foo == 3
    qux
else
    baz
end

# No
foobar = foo == 2 ? bar : foo == 3 ? qux : baz

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Sep 6, 2020

For Loops

  • For loops should always use in, never = or . This also applies to list and generator comprehensions (Already supported)
julia> str = """
       for i = 1:10
           #...
       end

       [foo(x) for x ∈ xs]
       """;

julia> print(format_text(str, always_for_in=true))
for i in 1:10
    #...
end

[foo(x) for x in xs]

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Sep 6, 2020

Comments

Before I write this bit out, @domluna can JuliaFormatter move or edit comments?
e.g. in this silly example, can it change from

looooooooong_variable = 10_000_000  # make this quite a big number so it takes up some space in the line

to

# make this quite a big number so it takes up some space in the line
looooooooong_variable = 10_000_000  

?

@domluna
Copy link
Owner

domluna commented Sep 7, 2020

Comment

Before I write this bit out, @domluna can JuliaFormatter move or edit comments?
e.g. in this silly example, can it change from

looooooooong_variable = 10_000_000  # make this quite a big number so it takes up some space in the line

to

# make this quite a big number so it takes up some space in the line
looooooooong_variable = 10_000_000  

?

Right now it just indents and dedents comments

inline comments like these are represented as their own node type so it probably wouldn't be too difficult to do this, I'm guessing it would depend on how general or niche this transform is, meaning is this done in all situations or only a handful?

@nickrobinson251
Copy link
Contributor Author

is this done in all situations or only a handful?

in cases where the inline comment would take the line length past the 92 char limit.

Here are the guidelines for comments:

  • inline comments should be separated by at least two spaces from the expression and have a single space after the #.

  • Only use inline comments if they fit within the line length limit
# Yes
# make this quite a big number so it takes up some space in the line
looooooooong_variable = 10_000_000  

# No
looooooooong_variable = 10_000_000  # make this quite a big number so it takes up some space in the line

  • Non-inline comment should be above the content to which it refers
# Yes:
# Number of nodes to predict. Again, an issue with the workflow order. 
# Should be updated after data is fetched.
p = 1

# No:
p = 1  # Number of nodes to predict. Again, an issue with the workflow order. 
# Should be updated after data is fetched.

@domluna
Copy link
Owner

domluna commented Sep 8, 2020

# Yes:
# Number of nodes to predict. Again, an issue with the workflow order. 
# Should be updated after data is fetched.
p = 1

# No:
p = 1  # Number of nodes to predict. Again, an issue with the workflow order. 
# Should be updated after data is fetched.

this case might be a pain to implement. I would be more comfortable leaving this up to the user.

@nickrobinson251
Copy link
Contributor Author

this case might be a pain to implement. I would be more comfortable leaving this up to the user.

Yeah, I think that's fair enough. Let's leave this one out :)

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Sep 9, 2020

By the way, the white-space rules for function definitions in BlueStyle allow quite a lot of flexibility.

In particular, it supports the idea you proposed here, Dom: jrevels/YASGuide#16 (comment))

Right now JuliaFormatter does this, which is consistent with BlueStyle

julia> str = """
       function long_name_of_function_because_i_am_writing_an_example(
           arg1, arg2, arg3, arg4, arg5, arg6,
       )
           # code
       end
       """;

julia> print(format_text(str))
function long_name_of_function_because_i_am_writing_an_example(
    arg1,
    arg2,
    arg3,
    arg4,
    arg5,
    arg6,
)
    # code
end

but the original code actually fit with in the line limit (once all the args were on that second line)
so BlueStyle would also allow you to leave the code as it was (in fact, that might be preferable), i.e.

julia> print(format_test(str; style=BlueStyle())  # hypothetical
function long_name_of_function_because_i_am_writing_an_example(
    arg1, arg2, arg3, arg4, arg5, arg6,
)
    # code
end

But then have the current behaviour if it'd go beyond the line limits to have all the args on one line e.g.
this current behaviour is still great

julia> str = """
       function long_name_of_function_because_i_am_writing_an_example(
           arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, arg9, arg10, arg11, arg12
       )
           # code
       end
       """;

julia> print(format_text(str))
function long_name_of_function_because_i_am_writing_an_example(
    arg1,
    arg2,
    arg3,
    arg4,
    arg5,
    arg6,
    arg7,
    arg8,
    arg9,
    arg10,
    arg11,
    arg12,
)
    # code
end

@domluna
Copy link
Owner

domluna commented Sep 18, 2020

@nickrobinson251 which ones are higher priority than others? I'm thinking it may be better to implement these incrementally instead of doing 1 big PR

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Sep 19, 2020

I'm thinking it may be better to implement these incrementally instead of doing 1 big PR

Yeah, that sounds better to me too.

which ones are higher priority than others?

I'd say top three priorities, in order, are

  1. Assignments using function calls should have the first open bracket on the same line as the assignment operator (the one discussed above)
  2. When using long-form functions always use the return keyword (but don't also add return to do-blocks)
  3. Separate keyword arguments from positional arguments with a semicolon

for context:
1 and 2 are high priority based on the changes the Formatter currently makes to code that follows BlueStyle (i.e. we want to remove cases where the Formatter makes style "worse" according to BlueStyle)

  • for 1, there's no setting of the formatter that doesn't get this "wrong"
  • for 2, there's no setting of the formatter that gets the return in fuction "right" without also getting something "wrong" (do-blocks)

3 is high priority based on this being the most common style reminder made in code-reviews on codebases following BlueStyle (at least the most common that the Formatter doesn't already help with, and based on my memory of code reviews), so first on the list of feature that'd be nice to have, i think.

@domluna
Copy link
Owner

domluna commented Sep 21, 2020

Assignments using function calls should have the first open bracket on the same line as the assignment operator (the one discussed above)

@nickrobinson251 does this also apply to short function definitions?

@nickrobinson251
Copy link
Contributor Author

Since we have "Only use short-form function definitions when they fit on a single line", this should never come up for short-form functions (i.e. if we're wondering how to put a short-form function over multiple lines, we should instead make it a long-form function)

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Sep 25, 2020

Using v0.9.1, this code gets changed to something not quite right (from https://github.com/invenia/LibPQ.jl/pull/197/files/0e908dfb097273fe1eb87050c4f0d99d071232ed#r494597229)

julia> str = """
           Dict(
               "options" => join(
                   imap(Iterators.filter(keep_option, connection_options)) do (k, v)
                       "-c k=(show_option(v))"
                   end,
                   " ",
               ),
           )
           """;

julia> println(format_text(str, BlueStyle()))
Dict(
    "options" => join(imap(Iterators.filter(keep_option, connection_options)) do (k, v)
        "-c k=(show_option(v))"
    end, " "),
)

Since in the changed version the join call breaks this rule

Function calls which cannot fit on a single line within the line limit should be broken up such that the lines containing the opening and closing brackets are indented to the same level while the parameters of the function are indented one level further. In most cases the arguments and/or keywords should each be placed on separate lines.

Instead, the ), should remain on its own line. (At least, i think that's what @iamed2 says should happen?)

@domluna
Copy link
Owner

domluna commented Sep 25, 2020

Using v0.9.1, this code gets changed to something not quite right (from https://github.com/invenia/LibPQ.jl/pull/197/files/0e908dfb097273fe1eb87050c4f0d99d071232ed#r494597229)

julia> str = """
           Dict(
               "options" => join(
                   imap(Iterators.filter(keep_option, connection_options)) do (k, v)
                       "-c k=(show_option(v))"
                   end,
                   " ",
               ),
           )
           """;

julia> println(format_text(str, BlueStyle()))
Dict(
    "options" => join(imap(Iterators.filter(keep_option, connection_options)) do (k, v)
        "-c k=(show_option(v))"
    end, " "),
)

Since in the changed version the join call breaks this rule

Function calls which cannot fit on a single line within the line limit should be broken up such that the lines containing the opening and closing brackets are indented to the same level while the parameters of the function are indented one level further. In most cases the arguments and/or keywords should each be placed on separate lines.

Instead, the ), should remain on its own line. (At least, i think that's what @iamed2 says should happen?)

So in an attempt to generalize this is it implied that if an argument spans multiple lines (the do block in this case), the args should spread automatically over multiple lines, regardless of margin?

@nickrobinson251
Copy link
Contributor Author

i'mma tag in @iamed2 on this one ^

@iamed2
Copy link

iamed2 commented Sep 26, 2020

So in an attempt to generalize this is it implied that if an argument spans multiple lines (the do block in this case), the args should spread automatically over multiple lines, regardless of margin?

Yep that's right

@domluna
Copy link
Owner

domluna commented Sep 28, 2020

woops apparently GH doesn't distinguish between issues and comments in issues

@domluna
Copy link
Owner

domluna commented Sep 29, 2020

746c456 disallows chained ternary

@domluna
Copy link
Owner

domluna commented Sep 29, 2020

94cb799 implements return nothing over return

@domluna
Copy link
Owner

domluna commented Sep 30, 2020

After a function definition, and before an end statement do not include a blank line.

What about in all cases except for modules?

#309

@nickrobinson251
Copy link
Contributor Author

What about in all cases except for modules?

#309

Sounds perfect 🎊

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Sep 30, 2020

This doesn't see quite right:

given this code that goes over the line limit

    df[:, :some_column] = [some_big_function_name(blahhh) for (fooooo, blahhh) in my_long_list_of_vars]

Ideally it'd be changed to

    df[:, :some_column] = [
        some_big_function_name(blahhh) for (fooooo, blahhh) in my_long_list_of_vars
    ]

But the BlueStyle breaks up the indexed on the left-hand side instead:

(using JuliaFormatter v0.10)

julia> str = """
           df[:, :some_column] = [some_big_function_name(blahhh) for (fooooo, blahhh) in my_long_list_of_vars]
       """;

julia> println(format_text(str, BlueStyle()))
df[
    :, :some_column
] = [some_big_function_name(blahhh) for (fooooo, blahhh) in my_long_list_of_vars]


julia> println(format_text(str))  # DefaultStyle, for comparison
df[:, :some_column] =
    [some_big_function_name(blahhh) for (fooooo, blahhh) in my_long_list_of_vars]

I think we just never want to break-up the left-hand side of the assignment operator =

@domluna
Copy link
Owner

domluna commented Sep 30, 2020

makes sense, that's what YASStyle does we can just dispatch that 1

fixed

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Oct 1, 2020

I'm trying out the formatter (v0.10.1) on a few repos, and it's pretty amazing. Great work!

Here's one case where i can't help but feel it's made the code less clean/clear... not sure what we can do about it

julia> str = """
function f()
    for i in 1:n
        @inbounds mul!(
            reshape(view(C, :, i), eye_n, k),
            reshape(view(B, :, i), eye_n, l),
            transpose(A),
        )
    end
end
""";

julia> println(format_text(str, BlueStyle()))
function f()
    for i in 1:n
        @inbounds mul!(
            reshape(view(C, :, i), eye_n, k), reshape(view(B, :, i), eye_n, l), transpose(
                A
            )
        )
    end
end

I wonder if the rule "if the function call doesn't fit on one line, then try to fit all the args on the next line, otherwise put them on a line each" that we use for function definitions should only apply to definitions, not also to callsites?
i.e. the rule that says

function long_name_of_function_because_i_am_writing_an_example(
    arg1, arg2, arg3, arg4, arg5, arg6
)
    # code
 end

is allowed/good should not be applied to the call, i.e. we'd still want

return_var = long_name_of_function_because_i_am_writing_an_example(
    arg1,
    arg2,
    arg3, 
    arg4,
    arg5,
    arg6,
)

Or at least, if it's written in this arg-on-multiple-lines way, the formatter shouldn't format it into the args-on-a-single-line (unless the whole function call fits on a single line i.e. f(args) fit on the same line)

What does @iamed2 think?

@domluna
Copy link
Owner

domluna commented Oct 1, 2020

That actually looks like a bug tbh

UPDATE: resolved as of v0.10.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants