Add a common rustfmt.toml for all diesel crates

[weiznich]

Formatting a new patch set is quite hard, so this should be automated. We could try to use rustfmt for this. This would require writing a own rustfmt.toml to get some kind of common style similar to the current one. I'm not sure if it's possible to match the current style using rustfmt.

[killercup]

We tried that last year in #197. Does rustfmt support a diff-friendly formatting yet? All my cries for trailing commas and consistent block ident are to reduce git churn and rightward drift, basically.

[weiznich]

I played little bit with some rustfmt flags. The result is something like the following:

struct A {
    a: i32,
    b: i32,
}

trait Foo {}

trait Bar {}

trait Baz {}

fn foo<T, S, W, SomethingLonger>() -> ()
    where T: Foo,
        B: Baz,
        W: Foo,
        SomethingLonger: Foo + Baz + Bar,
{
    println!("")
}

fn boom<T>() -> ()
    where T: Foo,
{
    println!("")
}

fn many_args<T, B>(
    a: i32,
    b: i32,
    c: i32,
    d: i32,
    e: i32,
    f: i32,
    g: i32,
    h: i32,
    i: i32,
    j: i32,
    k: i32,
    l: i32,
    j: i32
) -> i32
    where T: Foo,
        B: Bar
{
    println!("")
}

Notability to the current format:

• where is moved to the line of the first where bound. I don't think this is a problem because adding a new bound will only add a new line
• function argument list does not have a tailing comma. This is unfortunate, but there seems no option to change this

The following rustfmt.toml was used

where_trailing_comma=true
where_pred_indent="Tabbed"

fn_return_indent="WithWhereClause"
fn_arg_indent="Tabbed"
fn_args_layout="Block"

struct_trailing_comma="Always"
struct_lit_trailing_comma="Always"

match_block_trailing_comma=true
match_wildcard_trailing_comma=true

[sgrif]

This has been proposed before. It's still blocked on rust-lang-nursery/rustfmt#815.

[killercup]

Cool. Thanks for researching this!

The where on a new line is very unfortunate to me. Why are its entries (from the second on) indented by two levels? At least they got a trailing comma in there. ;)

Aside from that there are two edge cases that always seem to trip rustfmt up (I should probably open/have opened issues about that): Closures in method chains, and deeply nested types.

FTR, here's how I'd format a complex closure in a nested method chain (similar but not identical to how you did it #692):

let foobar_thingy_name: ImATypeName =
    syn::aster::from_generics(model.generics.clone()) // <- on a new line, doesn't have to, though
    .with_predicates(
        model.generics.lifetimes // <- yes this is on a new line as well
        .iter()
        .map(|l| syn::WherePredicate::RegionPredicate( // <- expr, so not in a block
            syn::WhereRegionPredicate {
                lifetime: l.lifetime.clone(),
                bounds: vec![insert.lifetime.clone()],
            }
        ))
    )
    .build(); // <- indented 1 level

And here are deeply nested types (also from #692):

type Lorem = (ColumnInsertValue::Expression(Version,
    AsExpression<
        <<Version as Expression>::SqlType as IntoNullable>::Nullable,
    >,
),);

type NullableColumn = AsExpression<
    <<Column as Expression>::SqlType as IntoNullable>::Nullable,
>;

which is to be read like

type Lorem = (ColumnInsertValue::Expression(Version,
    AsExpression<
        <<Version as Expression>::SqlType as IntoNullable>::Nullable,
//       '----------.----------'
//           inner-most type
//      '------------------------.-----------------------'
//             2nd level: don't break this into lines
    >
),);

If you can't put the inner (two) type levels on a line, you should probably introduce aliases.

If you can put any of these on one line (without the comments of course) and rustfmt them into something vey similar to what I wrote here, I'm happy :)

Update: Currently this ends with

$ rustfmt fmt.rs
Rustfmt failed at fmt.rs:1: line exceeded maximum length (maximum: 100, found: 136) (sorry)

and produces:

type Lorem = (ColumnInsertValue::Expression(Version, AsExpression< <<Version as Expression>::SqlType as IntoNullable>::Nullable, >, ),);

type NullableColumn = AsExpression< <<Column as Expression>::SqlType as IntoNullable>::Nullable, >;

fn main() {
    let generics = syn::aster::from_generics(model.generics.clone())
        .with_predicates(model.generics.lifetimes.iter().map(|l| {
            syn::WherePredicate::RegionPredicate(syn::WhereRegionPredicate {
                lifetime: l.lifetime.clone(),
                bounds: vec![insert.lifetime.clone()],
            })
        }))
        .build();
}

[weiznich]

The where on a new line is very unfortunate to me. Why are its entries (from the second on) indented by two levels? At least they got a trailing comma in there. ;)

The entries an indented by two levels, because the config tells rustfmt to do so. Changing where_pred_indent to "Inherit" moves the bounds to the same level as the where clause.
About the new line: At leas it does not introduce any additional lines into the diff of an patchset.

FTR, here's how I'd format a complex closure in a nested method chain (similar but not identical to how you did it #692):

The best I've got out of rustfmt is the following: (Not quite the same 😞 )

    let foobar_thingy_name: ImATypeName = syn::aster::from_generics(model.generics.clone())
        .with_predicates(model.generics
            .lifetimes
            .iter()
            .map(|l| {
                syn::WherePredicate::RegionPredicate(syn::WhereRegionPredicate {
                    lifetime: l.lifetime.clone(),
                    bounds: vec![insert.lifetime.clone()],
                })
            }))
        .build();

And here are deeply nested types (also from #692):

Rustfmt fails to format this type definitions. They remain unchanged without any message. (Even if the formatting is clearly broken.)

This has been proposed before. It's still blocked on rust-lang-nursery/rustfmt#815.

@sgrif If I understand correctly, the where keyword is only put on the same line as the return type to generate simpler diffs for patchsets?
The proposed variant does also not introduce any overhead to the generated diff, if the first bound is not removed. (But mostly in this cases you will also remove the corresponding type parameter and then also the function definition is changed.)

I think introducing a semi-automated variant, that get thinks "right" (whatever this means in this context) in most cases will reduced the overhead introduced by the style discussion on each pull-request. I think I've sufficiently shown that getting the style right is quite hard (Or maybe I don't spend enough attention on such things?)

(I do not suggest to introduce this now, but it should be some kind of goal for the next few releases.)

[killercup]

It seems to me that rustfmt and the formatting strike team are slowly but surely stabilizing on a code style. And even one I personally like! Things are looking good!

I think we can soon try this whole endeavor again, but with the default configuration.

[sgrif]

Agreed. There are a few more kinks being worked out, but I think we're going to move forward with the default configuration soon.