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

Alignment versus non-aligning indentation #8

Closed
joshtriplett opened this issue Sep 8, 2016 · 85 comments
Closed

Alignment versus non-aligning indentation #8

joshtriplett opened this issue Sep 8, 2016 · 85 comments

Comments

@joshtriplett
Copy link
Member

For a variety of cases, we should decide between alignment or non-aligning indentation. When breaking a line, should the item on the next line align with the item, or just have an additional level of indentation compared to the previous line? For instance:

fn alignment(arg: T,
             arg2: T2)

fn indentation(
        arg: T,
        arg2: T2)

let aligned = (value1
             + value2);
let indented = (
        value1
        + value2);
let indented2 = (
        value1
        + value2
);

let structval_aligned = S { field: value,
                            field2: value };

let structval_indented = S {
        field: value,
        field2: value,
};

function_call_aligned(param1,
                      param2);
function_call_indented(
    param1,
    param2);

This seems mostly orthogonal to indentation size, line length, and where exactly to break various types of lines.

I would propose avoiding alignment as a general principle. While alignment can sometimes improve readability, it produces significantly more noise in version control, since changes to one line can ripple outward into spacing changes on many lines. Alignment also tends to more quickly drift to the right edge of the screen, and produces worse results when wrapping lines close to a line-length limit.

In many cases, breaking the line and indenting before the first item provides the same benefit without the corresponding problem.

@strega-nil
Copy link

I agree, obviously :3

@nrc
Copy link
Member

nrc commented Sep 13, 2016

The terms we use in Rustfmt (which were terms of art elsewhere) are visual indent and block indent.

For block indent, there are two variations - either using a newline before the first argument or not:

fn some_function(arg1: Type1,
    arg2: Type2);

fn some_function(
    arg1: Type1,
    arg2: Type2);

We might want to consider some cases separately - currently Rustfmt use block indenting for struct literals and field/method chains, and visual indenting elsewhere. This brings up the issue of mixing, specifically, if you have a block indented item nested inside a visually indented item, do you block indent from the visual indent, or from the previous block indent?

In general, we must consider nesting carefully here.

I think we should probably decide on some of our guiding principles before dwelling too long here - this is a big and far-reaching decision and I think which choice we take comes down to how we prioritise issues:

@joshtriplett laid out arguments in favour of block indentation. It is also easier to implement and has fewer edge cases with nesting, etc.

However, visual indentation is (IMO) vastly more aesthetic in most situations, usually easier to read, and uses less vertical space (if adopting the second variation of block indentation).

It seems to me impossible to make a purely objective trade-off between these advantages, and so we must decide on the relative principle of our guiding principles first.

@solson
Copy link
Member

solson commented Sep 13, 2016

For myself, visual indent is more frustrating to read both in general and because it leads rustfmt into pathological edge cases more often. So, for me it's all downsides, but it seems that most of us at least agree that block indent has a simplicity advantage.

Of the guiding principles that have been discussed so far, I think these are the ones where block indent wins uncontroversially:

  • preventing right-ward drift
  • preserving diffs / compatibility with version control practices
  • ease of manual formatting
  • ease of automatic formatting for editors not using Rustfmt
  • ease of implementation
  • ease of re-implementation in other formatting tools
  • simplicity of formatting rules
  • simplicity for code generators outputting Rust code

And these are the ones where visual indent wins uncontroversially:

  • minimising vertical space

And these are the subjective ones I think could be argued both ways:

  • readability
  • scan-ability
  • consistency with formatting used in other languages
  • aesthetics

Many of the principles I listed for block indent are saying nearly the same thing, so a simple bullet point count score isn't fair, but I think it's still safe to say that block indent has more objective pros.

Unless a strong argument can be made that the subjective advantages of visual indent outweigh the advantages of block indent for the majority of the community, it seems logical to favour block indent.

@nrc
Copy link
Member

nrc commented Sep 13, 2016

Of the guiding principles that have been discussed so far

I think this is maybe jumping the gun a little bit - we should decide first which principles are important to us, and then make a decision here. The principles suggested so far are just suggestions, it may be that we decided that they should not be taken into account. And, as you point out, we should probably coalesce some of these principles.

@solson
Copy link
Member

solson commented Sep 13, 2016

@nrc Yeah, that's fair. I think putting them to the test like this might help us decide which are important, but it's good to note these are just suggestions for principles at this point.

@briansmith
Copy link

I would propose avoiding alignment as a general principle. While alignment can sometimes improve readability, it produces significantly more noise in version control, since changes to one line can ripple outward into spacing changes on many lines. Alignment also tends to more quickly drift to the right edge of the screen, and produces worse results when wrapping lines close to a line-length limit.

In many cases, breaking the line and indenting before the first item provides the same benefit without the corresponding problem.

The policy I've seen for clangformat in Google projects I work on seems to be "Prefer to align wrapped values unless that would cause the wrapped result to have more lines." That is, column width > minimum number of lines > aligned in terms of priorities. I think this makes the most sense.

@strega-nil
Copy link

Another argument in favor of block indent: visual indents is poor for accessibility, at least how it's currently used in rustc (@camlorn from irc):

camlorn eddyb: I have to specifically put the cursor there, read off the column number, then align it by counting spacces. [sic]

(note: camlorn is blind)

@retep998
Copy link
Member

retep998 commented Sep 25, 2016

For block indent, there are two variations - either using a newline before the first argument or not:

@nrc there is another variation where the ending parentheses goes on a new line, and is the style that I prefer and use.

fn some_function(
    arg1: Type1,
    arg2: Type2,
);

I remain strongly opposed to any attempts at making aligned indentation the standard style.

@joshtriplett
Copy link
Member Author

@retep998 In languages that allow a trailing comma on the last item, that's my preferred style as well for anything that doesn't fit comfortably on one line.

@retep998
Copy link
Member

retep998 commented Sep 25, 2016

I prefer my style because it is a very consistent style. Imagine if we formatted structs like this:

struct Foo { field1: Type1,
             field2: Type2 }
struct Foo { field1: Type1,
    field2: Type2 }
struct Foo {
    field1: Type1,
    field2: Type2 }

I don't see anyone clamoring to align structs like that, so why should we align functions like that?

@joshtriplett
Copy link
Member Author

@retep998 I think we're in complete agreement here. 😄

@ahicks92
Copy link

Not going to be very opinionated here, as I'm probably the sole blind contributor to the project and am capable of differentiating between what is good for me and what is good for the majority. That said I like the block style, though I'm not sure why the original comment is using 8 spaces.

The specific problem with aligning with the left paren as a blind person is that you can't tell if it's wrong without an explicit check. I put it right the first time. Then the function's name changes or what have you, and there's nothing reminding me to fix it. Then the PR gets a comment about it and I'm annoyed because that's annoying and also to me it's all senseless anyway.

Very few blind programmers use Braille displays for reading code. The way indentation is exposed (if you bother to have it on) is by announcing the number of spaces at every change. Python is accessible, but indentation isn't about being pretty, it's about being semantical, for lack of a better word. I use it because it's helpful in getting closing braces right and sighted people will flat out refuse to read code without it in some cases, but otherwise I wouldn't even bother with it.

I also miss things like spaces after // or ///, spaces around ->, etc. And then there's the ones I absolutely don't mean to miss like spaces before {. And the ones that make no intuitive sense to me, like (.. vs. { .. }.

I suppose it would be worse if I had always been fully blind, but I had enough that I was able to get some idea of why code layout is how it is. Otherwise, this conversation would probably be going quite, quite differently...

@joshtriplett
Copy link
Member Author

@camlorn To me, that's a substantial argument in favor of block indentation and against visual indentation. I appreciate your feedback and perspective.

Even when it's possible to see the misaligned visual alignment at a glance, people often send patches that don't update the alignment or that copy/pastes the alignment for another function, and often that issue gets missed, resulting in code that no longer has any sensible alignment. Together with the point that it's even more painful to deal with that alignment without visual feedback, that makes me even more inclined to avoid visual alignment.

I've also proposed in #4 that we take non-visual interfaces such as speech into account as a guiding principle. That doesn't mean it'll always be the highest priority consideration, but we should always take it into account.

@briansmith
Copy link

@camlorn Isn't rustfmt itself an accessibility tool that obviates the need to check the formatting of code before submitting patches for review? That is, you'd just cargo fmt before git add and git commit and not need to worry at all? That is, as far as screen readers are concerned, it seems like the onus should be on the project to ensure that cargo fmt works correctly, and not necessarily on the default styling rules. Otherwise, if we optimized the default rules for screen readers then I think we'd have, for example, one line per statement, no matter how long it is.

@ahicks92
Copy link

@briansmith
Well, as far as I know rustfmt doesn't run on the compiler itself.

I actually don't do one-line statements when they get long. The current limit of 100 chars is about right. I'll sometimes break them at what you would consider odd places, but there is a cost to be payed to navigate from the left of a long line. I can't click in the middle like you can, so the navigation algorithm is roughly O(n) on characters. I get move up/down by line, next/previous "word" (but what that is varies depending), next/previous character, and sometimes coding specific stuff that I personally never use. To be concrete, I'd do (and did do in my recent PR) something like this:

really_really_long_function(
    really_big_expression1+really_big_expression2,
);

Or even:

really_really_long_function(
    really_big_expression1
    +really_big_expression2,
);

(by putting the + at the beginning of the line, it is easier to tell that it's continuing the previous one; if this isn't feasible or simply if I remember, I'll indent it a bit more).

But the thing here is, blindness isn't a standard modality. What I do isn't what the next blind person does, and I'd be really surprised if you could find very many "You're blind so you consider xyz to be good code formatting" statements that actually generalize. Lining up with the left paren being hard and not making much sense is one of the few that does, and even then you can probably find someone who used to program with full vision that still bothers with it, even though they're blind now.

@briansmith
Copy link

Well, as far as I know rustfmt doesn't run on the compiler itself.

I don't know what you mean here. Are you referring to the compiler's error messages? I guess I was thinking that anytime formatting mattered, one would run cargo fmt or equivalent, like I do when dealing with Go code.

In my message, I was specifically referring to the use case you mentioned where you run into difficulty because code reviewers demand code be formatted a certain visual way, which is hard for you to do. My thinking, based on my own experience working on Go code, is that automatic formatting tools exactly solve that problem. Maybe there are other problems that I am overlooking.

I didn't mean to imply that there's one perfect way to format code optimally for blind people. But, I'd also say that if rustfmt formatted code like the styles you suggested, then I just wouldn't use rustfmt in my projects, because they are very sub-optimal styles for me. To put my earlier statement in another way, what I meant is that certain optimizations for non-sighted people are going to have a negative effect on sighted people, and vice-versa.

@retep998
Copy link
Member

retep998 commented Sep 27, 2016

@camlorn I too prefer the style where, when splitting a binary expression across multiple lines, the operator is put at the beginning of the next line. I also indent the continuation lines as well.

As for block indenting functions, which of these two do you prefer? I've been using the former in winapi quite a bit to save vertical space (it really helps when there are literally hundreds of function declarations in a file), but I've been considering switching to the latter.

pub fn CreateFileW(
    lpFileName: LPCWSTR, dwDesiredAccess: DWORD, dwShareMode: DWORD,
    lpSecurityAttributes: LPSECURITY_ATTRIBUTES, dwCreationDisposition: DWORD,
    dwFlagsAndAttributes: DWORD, hTemplateFile: HANDLE,
) -> HANDLE;
pub fn CreateFileW(
    lpFileName: LPCWSTR,
    dwDesiredAccess: DWORD,
    dwShareMode: DWORD,
    lpSecurityAttributes: LPSECURITY_ATTRIBUTES,
    dwCreationDisposition: DWORD,
    dwFlagsAndAttributes: DWORD,
    hTemplateFile: HANDLE,
) -> HANDLE;

@nrc
Copy link
Member

nrc commented Sep 27, 2016

Several of @camlorn's points seem to come down to the intended work flow around the default styles (this is somewhat discussed in #4) - how heavily do we want to weight ease of use without Rustfmt? I think if we assume that the programmer will do no manual formatting at all (i.e., always use Rustfmt or some other tool) we will have quite different constraints to if we want a style that can be easily enforced by both a user and a tool. As I wrote on #4, I think we should try and have a style that is easy to write manually, but not at the expense of having a style that is less easily readable or less pretty.

@retep998
Copy link
Member

According to reports from other people, rustfmt can't format the internals of macro invocations, and because winapi is mostly macros, that means I effectively can't use rustfmt for winapi, even if it could achieve the style I desired. So for me, having a style that can be manually done very easily is critically important. Block indentation is incredibly easy to do manually.

@joshtriplett
Copy link
Member Author

On September 26, 2016 5:41:03 PM PDT, Peter Atashian notifications@github.com wrote:

@camlorn I too prefer the style where, when splitting a binary
expression across multiple lines, the operator is put at the beginning
of the next line. I also indent the continuation lines as well.

As for block indenting functions, which of these two do you prefer?
I've been using the former in winapi quite a bit to save vertical
space, but I've been considering switching to the latter.

pub fn CreateFileW(
   lpFileName: LPCWSTR, dwDesiredAccess: DWORD, dwShareMode: DWORD,
lpSecurityAttributes: LPSECURITY_ATTRIBUTES, dwCreationDisposition:
DWORD,
   dwFlagsAndAttributes: DWORD, hTemplateFile: HANDLE,
) -> HANDLE;
pub fn CreateFileW(
   lpFileName: LPCWSTR,
   dwDesiredAccess: DWORD,
   dwShareMode: DWORD,
   lpSecurityAttributes: LPSECURITY_ATTRIBUTES,
   dwCreationDisposition: DWORD,
   dwFlagsAndAttributes: DWORD,
   hTemplateFile: HANDLE,
) -> HANDLE;

I vastly prefer the latter, because it doesn't result in rippling changes when adding or removing a parameter.

@joshtriplett
Copy link
Member Author

On September 26, 2016 3:24:53 PM PDT, Brian Smith notifications@github.com wrote:

@camlorn Isn't rustfmt itself an accessibility tool that obviates the
need to check the formatting of code before submitting patches for
review? That is, you'd just cargo fmt before git add and git commit and not need to worry at all? That is, as far as screen readers
are concerned, it seems like the onus should be on the project to
ensure that cargo fmt works correctly, and not necessarily on the
default styling rules. Otherwise, if we optimized the default rules for
screen readers then I think we'd have, for example, one line per
statement, no matter how long it is.

Only if you systematically run it on every commit, and always make a separate commit if rustfmt itself changes its style.

@nrc
Copy link
Member

nrc commented Sep 27, 2016

Only if you systematically run it on every commit

This seems like a reasonable workflow (we currently do it with make tidy), I would expect to be running Rustfmt on every commit to Rust at some point in the not too distant future and would encourage other projects to run it on their CI.

@ahicks92
Copy link

@briansmith
Currently, running rustfmt against the Rust compiler can't be done, or so I was told. In my own projects, I'll do whatever I like and expect people contributing to try to at least be consistent, without putting much weight on it (try doing a code review of formatting as a blind person...no). SO the only place rustfmt is helpful is projects that aren't mine and aren't Rust tools that build before Cargo. How helpful it is in practice, on those projects where It can be used, I don't know. I'm not overly familiar with it. I suppose I could do my own thing but be consistent via rustfmt, if it were sufficiently configurable.

I'm also not sure if I'm speaking about all Rust projects or specifically stuff internal to the official rust-lang organization here. I haven't actually figured out what this repository is for, and only know it exists because I was mentioned here.

@retep998
I don't have an impression. The first makes it easier to get to the body (procedure is search for fn and/or pub fn at beginnig of line, arrow down). The second makes it much less likely that I'd miss a parameter. Given that making sure I don't miss parameters is something I'm already good at, this isn't a huge deal.

The point about rippling changes is a good one, though in this case it's winapi, so they probably never change.

@frewsxcv
Copy link
Member

Currently, running rustfmt against the Rust compiler can't be done, or so I was told.

rustfmt gets run on the compiler quite frequently

@ahicks92
Copy link

@frewsxcv
Interesting. How automatic is this? How far from make fmt are we?

@nrc
Copy link
Member

nrc commented Sep 27, 2016

Interesting. How automatic is this? How far from make fmt are we?

Not at all, and quite a way off. We basically need to format the whole repo before it is practical to have each commit formatted. It is the long term plan though. Needs Rustfmt to be a bit better and to some extent decide on these style guidelines.

@ahicks92
Copy link

@nrc
Shame. That would have made my life much easier.

@allan-simon
Copy link

allan-simon commented Oct 13, 2016

I strongly agree that using

pub fn CreateFileW(
    lpFileName: LPCWSTR,
    dwDesiredAccess: DWORD,
    dwShareMode: DWORD,
    lpSecurityAttributes: LPSECURITY_ATTRIBUTES,
    dwCreationDisposition: DWORD,
    dwFlagsAndAttributes: DWORD,
    hTemplateFile: HANDLE,
) -> HANDLE;

or

my_function_call(
    param1,
    param2,
)

has some serious advantage in term of being diff friendly

  • renaming the function only change the function name , and can only be about the function name
-  my_function_call(
+ my_new_function_call(
  • adding a parameter is only adding a line (as rust permits you to have trailing , even for last element)
+   new_parameter,
  • removing a parameter is only removing a line

with this you can have upfront a pull request changing the function name , and a separate pull request changing the parameter name , and one removing a parameter without any causing conflict to the others.
While indentation that follow the size of the function name, or indentation where you can put several items on the same line would have caused conflict. it's a strategy that really helped my team of 20 programmers to make the process of codereview more enjoyable (easier to know in one glance what the commit modified), and reduce to a minimun the chance that you got a conflict only because of how the code is organized.

last but not least, it makes a very very simple rules on how to indent, if you need multiline

  • an opening "block" is the only thing in a line
  • the list of inner items are one by line , indented by one
  • the closing block is alone on the line and indented like the opening

and you can apply it to array initialization, function call, function signature etc. etc.

of course the indentation is a mean to readability and not an end by itself

@bruno-medeiros
Copy link

However, visual indentation is (IMO) vastly more aesthetic in most situations, usually easier to read, and uses less vertical space (if adopting the second variation of block indentation)

@nrc , do find code like this easier to read? -> https://github.com/jonathandturner/rls/blob/master/src/test/mod.rs#L94 😧
IMO, it doesn't look easier to read or aesthetic at all, quite the opposite. I think block indentation would be better.

@nrc
Copy link
Member

nrc commented Nov 15, 2016

Further to @joshtriplett's comment, we decided that the outcome of this process should be an RFC, but to summarise the discussion here, rather than to specify Rustfmt.

We decided that we liked the use of block formatting as a principle, and that while there may be exceptions, these should be well-motivated and rare. We did not agree on how binding this principle should be, although in retrospect I'm not sure how much that matters.

We decided that since conversation is ongoing here, we should leave this issue open for now.

@nrc
Copy link
Member

nrc commented Nov 15, 2016

Several people have asked for more code. I agree that would be good to have. Rustfmt can be configured to format some things using block formatting. However, there are a lot of details to resolve even once a preference for block formatting has been decided and I'm not sure that Rustfmt can at the moment format a file in a way that would make block-formatting advocates entirely happy (or given the different choices, whether it would be indicative of a final style).

@nrc
Copy link
Member

nrc commented Nov 15, 2016

@aturon most of the issues you mention are open questions in my mind. This makes decision making difficult and even making code examples tricky. I feel that perhaps the best thing to do would be to consider some of the trickier scenarios (function calls, method chains) in their own right and come up with a detailed plan there, rather than try and push harder for a general rule.

@nikomatsakis
Copy link

nikomatsakis commented Nov 15, 2016

Hmm. So, I've found that when I am using emacs, I prefer visual indent, because it happens automatically and it looks nicer. When I'm not using emacs, I tend to block indent, because I can do it more easily and it looks "nice enough". This seems to back up @nrc's point that automation is key for visual indent. It's also true that I sometimes insert newlines to make things look nice, which probably backs up the contention that visual indent is somewhat harder to do automatically.

That said, I think the details of block indent matter quite a bit. In particular, I think that keeping the nested structure is crucial. For example, take this iterator chain I was just hacking on. Formatting using visual indent, the nested structure is pretty clear:

    pub fn iter_parents<'a>(&'a self) -> impl Iterator<Item=&'a Environment>+'a {
        let parent = self.parent.as_ref();
        Box::new(once(self)
                 .chain(
                     parent.into_iter()
                           .flat_map(|e| e.iter_parents())))
            as Box<Iterator<Item=&'a Environment>>
    }

But when I run it through rustfmt, which uses a simple block indent, the result is nigh unreadable to me (leaving aside the fact that it mangles impl Trait into TODO, which btw is super annoying):

    pub fn iter_parents<'a>(&'a self) -> impl TODO {
        let parent = self.parent.as_ref();
        Box::new(once(self).chain(parent.into_iter()
            .flat_map(|e| e.iter_parents()))) as Box<Iterator<Item = &'a Environment>>
    }

That said, a variant of block indent that preserves nesting might be fine. Whenever I've done "block indent" manually, my rule is usually something like: "if the arguments to a call are too long for one line, put a newline after the (, put them on separate lines, and indent by one level". That would leave us with something like this, which seems tolerable:

    pub fn iter_parents<'a>(&'a self) -> impl Iterator<Item=&'a Environment>+'a {
        let parent = self.parent.as_ref();
        Box::new(
            once(self).chain(
                parent.into_iter()
                      .flat_map(|e| e.iter_parents())))
            as Box<Iterator<Item=&'a Environment>>
    }

My take-away from this, I think:

  • I can go either way on visual vs block indent
  • But whatever we choose, I think we must preserve nesting structure

@Alex-PK
Copy link

Alex-PK commented Nov 15, 2016

I know somebody dislike the single ) on a single line, but I would change this:

pub fn iter_parents<'a>(&'a self) -> impl Iterator<Item=&'a Environment>+'a {
        let parent = self.parent.as_ref();
        Box::new(
            once(self).chain(
                parent.into_iter()
                      .flat_map(|e| e.iter_parents())))
            as Box<Iterator<Item=&'a Environment>>
    }

into this:

pub fn iter_parents<'a>(&'a self) -> impl Iterator<Item=&'a Environment>+'a {
    let parent = self.parent.as_ref();
    Box::new(
        once(self).chain(
            parent
                .into_iter()
                .flat_map(|e| e.iter_parents())
        )
    ) as Box<Iterator<Item=&'a Environment>>
}

This way you see where the chain() ends, and that as Box<...> is AFTER Box::new and not one of its parameters.

To recap: a closing parenthesis (of any type) should be on the same indentation level as the line opening it (unless it's on the same line, obviously)

@nikomatsakis
Copy link

nikomatsakis commented Nov 15, 2016

@Alex-PK actually, I like that better.

(edit: changed wording to clarify meaning)

@strega-nil
Copy link

@Alex-PK That's similar to Rustic style :)

@nical
Copy link

nical commented Nov 25, 2016

A bit of feedback from 5 years of hacking on a large code base using visual indents (Gecko):

  • Visual indents conflict with the line length limit very often (*).
    • You end up having to choose between respecting one of the two rules.
    • You have to choose between the different ways you are going to compromise.
    • Not everybody agrees about which rule to break and which way to break it (super annoying when you settle for one compromise and the reviewer asks you to reformat the code making a different compromise, when the problem has been dealt with 5 different ways in the same file).
    • Overall, the shape of the code is not as uniform as it would be if rules wouldn't conflict so often.
    • Encourages compromising on naming to avoid dealing with the above.
  • Misaligned code all over the place due to search-and-replace types of changes.

It's still possible to run into the line length limit with block indent but much much less so in practice.
I got used to the aesthetics of visual indents (at the end of the day we'll all get used to the look of a certain imposed style), I don't particularly like it but I don't mind it. I do mind the cognitive overhead of dealing with the conflicting rules so often and the resulting inconsistencies. If I could change Gecko's coding style I would change a lot of it, but if I could change only one thing, it would be moving from visual indents to block indents.

An argument has been made that the style guide should assume everybody will use rustfmt for everything (reducing part of the cognitive burden concerns), which I am sceptical about but I hear it (and hope it will come to be true).

(*) This type of thing:

            // add a few indents because you are in a branch in a loop inside a function...
            some_complicated_object.nested_thing.properly_explicative_method_name(foo.computed_thingamagingy(world.thing),
                                                                                  &something_else.properly_named_variable.inner_size);

@Keats
Copy link

Keats commented Nov 30, 2016

Just to comment on @brson formatting, I would format it that way:

let messages = vec![
    Message::new("initialize", vec![
        ("processId", "0".to_owned()),
        ("rootPath", root_path),
    ]),
    Message::new("textDocument/definition", vec![
        ("textDocument", text_document),
        ("position", position),
    ]),
];

Basically I do the same thing @Alex-PK does: matching opening brackets to the closing ones in terms of indentation.
More lines than the visual indentation but (imo) readable. I'm having a lot of trouble seeing the aesthetic benefits of the visual indentation, it just looks unreadable to me.

@thorbenk
Copy link

Something that @nical has said was also on my mind:
Visual indentation can encourage the use of bad (that is: short and non-descriptive) variable names, just to combat rightward drift across the column limit.

@scottmcm
Copy link
Member

scottmcm commented Dec 6, 2016

An advantage of visual indentation that I haven't seen mentioned is in lining up code that's almost the same. For instance:

    let node_count = count_of_nodes_inner(range.left)
                   + count_of_nodes_inner(range.right);

// compare to

    let node_count = count_of_nodes_inner(range.left)
        + count_of_nodes_inner(range.right);
    let node_count = 
        count_of_nodes_inner(range.left)
        + count_of_nodes_inner(range.right);

Of course that's only relevant when binary operators are involved. (Or other separators, like lining up |s in yacc grammars.) Was a decision made on where operators go when splitting lines?

@strega-nil
Copy link

@scottmcm I do the last option, personally.

@Alex-PK
Copy link

Alex-PK commented Dec 6, 2016

@scottmcm This can be solved by putting the operator at the end of the line:

let node_count = 
        count_of_nodes_inner(range.left) +
        count_of_nodes_inner(range.right);

But, to be honest, I think it's nice when it happens, but it doesn't happen often. :)

@scottmcm
Copy link
Member

scottmcm commented Dec 7, 2016

@Alex-PK Yeah, I wasn't sure if the operator position for wrapping was decided.

I just thought I'd add it to the list. I would not be surprised if the answer is "we have good abstraction and inference; don't even copy one line". There's certainly an argument that it should be [range.left, range.right].map(count_of_nodes_inner).sum() instead. And it's not something that happens in structs or signatures anyway...

Maybe it's only useful as an explicit counterexample to "Always avoid rivers": sometimes code makes columns of similarity because it's similar, and that similarity being visually apparent is virtuous.

@retep998
Copy link
Member

retep998 commented Dec 7, 2016

Also adding to the list of alternatives...

let node_count
    = count_of_nodes_inner(range.left)
    + count_of_nodes_inner(range.right);

@Alex-PK
Copy link

Alex-PK commented Dec 11, 2016

@scottmcm sorry, I missed your comment regarding the operator position.

I prefer the operator at the end of the line, but I have no strong opinion on that.

@joshtriplett
Copy link
Member Author

I'd like to propose that we separate "operator at beginning of second line / end of first line" into a separate discussion, and refocus this on "block indent versus visual indent". We seem to have developed a reasonable consensus for block indent, with the possibility of making exceptions on a case-by-case basis. That consensus seems settled enough that we might be able to resolve this.

@nrc
Copy link
Member

nrc commented Mar 8, 2017

SGTM. I will close this issue as resolved. I don't think we need to take any further action.

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