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

Convention about empty lines #57

Closed
KalitaAlexey opened this issue Feb 1, 2017 · 42 comments
Closed

Convention about empty lines #57

KalitaAlexey opened this issue Feb 1, 2017 · 42 comments
Labels

Comments

@KalitaAlexey
Copy link

KalitaAlexey commented Feb 1, 2017

Hi everybody.

It looks like there is no any convention about empty lines.

I like to separate all function with one empty line.

fn foo() {}

fn bar() {}

I like to separate Control Flow Statements with one empty line.

fn foo(x: i32) -> i32 {
    let result = calculate(x);

    if result > 0 {
        result
    } else {
        0
    }
}

I like to separate blocks with one empty line.

fn foo() {
    {
        do_something();
        do_something_else();
    }

    {
        do_something_different();
        do_something_completely_different();
    }
}

I like to separate unrelated pieces with one empty line.

fn foo() {
    let mut x = create_x();
    x.do();
    x.do_something_else();

    let mut y = create_y();
    y.do();
    y.do_something_else();
}

I think we should have some convention about it.

Thanks,
Alexey

@gsingh93
Copy link

gsingh93 commented Feb 1, 2017

I think we should only define a convention for top level statements inside modules, i.e. one line between functions, struct definitions, impls, etc. I also think we should require the newline between method declarations inside impls. Of course, top level statements like use and extern would be grouped together (I think another issue addresses how to do this grouping). I'm not sure if this rule should apply to traits, or at least to traits with methods that don't have a body.

I don't think we should have any other requirements, and leave how to use newlines inside blocks and between control flow statements up to the user.

@nrc
Copy link
Member

nrc commented Feb 1, 2017

I have been somewhat reluctant to address this in rustfmt, where we treat newlines as significant whitespace and preserve it in the same way as comments. The reason for this is that People often use newline semantically - no lines between related statements, a blank line between separate groups of statements, multiple blank lines to enforce an even greater separation. Likewise, between items at module scope, if there are multiple single line, related functions there might be no newlines. Mostly functions are separated by one line, sometimes grouped using multiple lines.

Furthermore, it is really easy to fix manually - you just delete them, its not like you have to re-lay out code.

There seem to be some cases we can safely forbid, e.g., blank lines at the start of end of a block.

For other uses I see a few options:

  1. continue to do preserve all newlines
  2. preserve 0, 1, or 2 blank lines, e.g., a 5 line gap would be reduced to 2.
  3. preserve 0 or 1 blank lines, e.g., a 5 line gap would be reduced to 1.
  4. enforce 1 line between all items and do preserve function bodies (or reduce multiple lines to 1 inside function bodies).

I prefer 1 or 3 - I think 4 is too strict, and 2 is unnecessary.

@KalitaAlexey
Copy link
Author

@nrc,
I like forbidding of blank lines at both the start and the end of a block.

I am up to 3 variant.

@joshtriplett
Copy link
Member

joshtriplett commented Feb 1, 2017 via email

@chriskrycho
Copy link

chriskrycho commented Feb 3, 2017

I'd strongly prefer option 2 (edit: if we transform anything at all; [1] is also fine), because I do (borrowing from Python) tend to use 2 blank lines at the top level, and especially between different sections. I find extra spaces very helpful for visually separating logically/semantically distinct sections of the code, especially the use statements, distinct data structures and their implementations, and standalone functions. I would be really frustrated to have rustfmt collapse those.

Example

//! Some module docs
//!
//! More module docs. All the docs. Docs and docs and docs.


// First-party dependencies
use std::String;
use std::borrow::Borrow;

// Third-party dependencies
use foo::Foo;


/// Hey look, a `Quux`.
pub struct Quux {
    // ...
}

impl Quux {
    /// Give me a `Quux`!
    pub fn new() -> Quux {
        // ...
   }
}

impl Borrow<T> for Quux {
  fn borrow(&self) -> &T {
    // ...
  }
}


/// This is a `Bar`.
pub enum Bar {
    VariantA,
    VariantB(i32),
    VariantC { q: u64 },
}

impl Bar {
    // ...
}


/// Hey, sometimes we have standalone functions, too
pub fn baz(bar: Bar) -> Quux {
    // ...
}

@nrc
Copy link
Member

nrc commented Feb 9, 2017

I used to use 2 line spaces as significant, however, my feeling more recently that this is too subtle to be useful and that if I want to separate groups of items, then they should be in different files, or I should use a comment to separate. Having said that, I am still somewhat sympathetic to this approach.

We could have option 3, with option 2 as a config option. I think this is quite suitable as a config option - it seems reasonable to disagree with the default, and it doesn't make the code unfamiliar to change the value.

@tshepang
Copy link
Member

tshepang commented Feb 9, 2017

Though I come from Python, I've come to appreciate that there is no fussing about "is it one space, or is it two" in Rust. I see no readability issue, and I've even come to visually appreciate the compactness the one-space approach that promotes.

Looking at @chriskrycho preference, I be sad... way too sparce, especially with those doc comments.

@nrc
Copy link
Member

nrc commented Mar 26, 2017

Entering FCP:

  • by default Rustfmt will reduce whitespace between items to a single blank line. If items are not separated by blank lines, they will not be inserted.
  • items on the same line will be put on separate lines with no blank lines between them
  • Rustfmt should be configurable to specify the minimum and maximum number of line breaks allowed.

@solson
Copy link
Member

solson commented Mar 27, 2017

by default Rustfmt will reduce whitespace between items to a single blank line. If items are not separated by blank lines, they will not be inserted.

To be clear, this also applies to statements, right? E.g. if the max is set to 1 blank line, we should change the following:

fn foo() {
    bar();
    baz();


    quux();
}

Into this:

fn foo() {
    bar();
    baz();

    quux();
}

@jplatte
Copy link

jplatte commented Mar 27, 2017

Rustfmt should be configurable to specify the minimum and maximum number of line breaks allowed.

The minimum number of (consecutive) line breaks allowed? Is this talking about a minimum between certain elements, or a rule like "a blank line must always have another blank line before or after it"? Because the former sounds like it would have to be multiple options for different contexts, and the latter would be very weird IMHO.

@nrc
Copy link
Member

nrc commented Mar 28, 2017

To be clear, this also applies to statements

We could do. What do you think? I'm a little twitchier about statements than items, just because people are more likely to put spacing in, but I think it makes sense to apply the same treatment.

@nrc
Copy link
Member

nrc commented Mar 28, 2017

The minimum number of (consecutive) line breaks allowed?

Yeah, so you'd get the following formatting with the various settings:

// 0
fn foo() {} fn bar() {}

// 1
fn foo() {}
fn bar() {}

// 2
fn foo() {}

fn bar() {}

The default would be 1.

I don't think different contexts would require different values. We might not want to apply this to statements, only to items, or have different values for items and statements.

@solson
Copy link
Member

solson commented Mar 28, 2017

With regards to settings, to match what I do today, I would force one space between items (1...1) but zero or one space between statements (0...1).

Given these items (EDIT: made them multi-line. My comment below addresses single-line items):

fn foo() {
    // ...
}
fn bar() {
   // ...
}


fn baz() {
   // ...
}

I would produce:

fn foo() {
    // ...
}

fn bar() {
    // ...
}

fn baz() {
    // ...
}

But given these similar statements:

foo(); bar();


baz();

I would produce:

foo();
bar();

baz();

Basically, I find value in user-controlled statement spacing but not in user-controlled item spacing. Items are usually clearly separated by doc comments or impl grouping instead. You can also demarcate explicit sections with plain top-level comments between items. Statement grouping is more fluid.

@solson
Copy link
Member

solson commented Mar 28, 2017

Some edge cases:

fn foo() {

    bar();
}
fn foo() {
    bar();

}

I would remove these end-of-block and start-of-block blank lines in all cases.

@nrc
Copy link
Member

nrc commented Mar 28, 2017

It is fairly common for people to have large numbers of small (one line) functions without line breaks (e.g., signatures in traits). I think we should support that (probably by default).

@solson
Copy link
Member

solson commented Mar 28, 2017

It is fairly common for people to have large numbers of small (one line) functions without line breaks (e.g., signatures in traits). I think we should support that (probably by default).

Fair point. A more complete rule I follow for items is this:

  1. place exactly one blank line between a multi-line item and its adjacent items
  2. place no blank lines between single-line items

Note that the following are considered multi-line items:

#[inline(always)]
fn foo() {}
/// Documentation!
fn bar() {}

But the following is allowed:

fn simple() {}
fn helper() {}
fn functions() {}

Here is a general mix to show what my rule entails:

/// Documentation!
fn foo() {}

fn bar() {}
fn baz() {}

fn multi_line() {
   // ...
}

fn quux() {}
fn xyzzy() {}

Note that you do place a blank line between a multi-line item and a single-line item to avoid a crowded look.

@nrc
Copy link
Member

nrc commented Mar 28, 2017

A more complete rule I follow for items is this:

At this point I'd rather just allow some flexibility for the user. I think some people do like to use blank space semantically here and that seems like something we could allow without too much bother.

@solson
Copy link
Member

solson commented Mar 28, 2017

To elaborate on some of my reasoning, I like my style to avoid this anti-pattern I've seen in novice code:

/// Group documentation about `a`, `b`, and `c`.
fn a() {}
fn b() {}
fn c() {}

Since rustdoc will only apply the documentation to a. This fact is made more obvious when you use my rules:

/// Group documentation about `a`, `b`, and `c`.
fn a() {}

fn b() {}
fn c() {}

@solson
Copy link
Member

solson commented Mar 28, 2017

Another place this comes up is with fields or variants, which I treat the same way as items:

// OK
struct Foo {
    /// Docs for `a`
    a: A,

    /// Docs for `b`
    b: B,
}

// OK
struct Foo {
    a: A,
    b: B,
}

// I would reject this
struct Foo {
    /// Docs for `a`
    a: A,
    /// Docs for `b`
    b: B,
}

@solson
Copy link
Member

solson commented Mar 28, 2017

At this point I'd rather just allow some flexibility for the user. I think some people do like to use blank space semantically here and that seems like something we could allow without too much bother.

Every time we talk about this kind of flexibility, what I mostly think is "this is yet another thing I have to manually check for in code review".

There's a definite trade off between what is automatic and what is flexible, and of course I like a moderately flexible treatment for spacing in statements, but I would like as much as possible to make things like this never exist in rustfmt's output:

/// Documentation.
fn foo() {
    // ...
    // ...
    // ...
}
/// Documentation.
fn bar() {
    // ...
    // ...
}

Because I consider this level of density harmful, and it's easier for me to tell contributors to just run rustfmt. If this isn't handled automatically I need to have my own style guide or risk arguing about style in code reviews. I'd rather never even mention style in code reviews.

So, I'm occasionally accepting but quite wary of flexibility. It weakens the benefit I get from using rustfmt.

@joshtriplett
Copy link
Member

I agree. Enforcing exactly one blank line (two newlines) between top-level items seems fine. And requiring that statements appear on separate lines and have either zero or one blank line between them (with no blank lines at the start or end of a block) also seems fine.

I'd rather rustfmt not eliminate blank lines between statements, but reducing multiple blank lines to one and ensuring that multiple statements don't appear on the same line seems fine.

@nrc
Copy link
Member

nrc commented Mar 28, 2017

I am somewhat swayed by @solson's code review argument. However, I feel personally that the example of functions close together would not warrant an r- or even a comment and so I don't feel that it is so important for rustfmt to deal with it.

I worry about the details here, for example the enum example:

struct Foo {
    /// Docs for `a`
    a: A,
    /// Docs for `b`
    b: B,
}

That seems totally reasonable to me. However if A or B got more complicated, then I would insert a blank line. Likewise for functions, I would accept one-liners. But if they had say 10 attributes or 20 lines of doc comments, then I'd want a blank line.

I fear there are many such edge cases and it is an area where users are going to want the flexibility. It is also somewhere where the benefit of having Rustfmt do it for you is not so large - it is much easier to hit enter than to align a bunch of expressions. And of course, if a project maintainer does want to be super-strict they can use a config file and tighten up the defaults.

@phaylon
Copy link

phaylon commented Mar 28, 2017

I do tend to have empty lines at the beginnings of blocks, for example when the first block element is commented.

@joshtriplett
Copy link
Member

@phaylon Can you provide an example of that?

@solson
Copy link
Member

solson commented Mar 28, 2017

Presumably rustfmt would notice the fact that there's a commented block, so it wouldn't remove whitespace in that case.

@phaylon
Copy link

phaylon commented Mar 28, 2017

@joshtriplett One I could find quickly:

fn parse(&self, input: Input<'i>) -> ParseResult<'i, &'i str, NoWhitespace, !> {

    // read to end if nothing but whitespace
    let pos = input.slice().char_indices()
        .find(|&(_, c)| !is_whitespace(c))
        .map_or_else(|| input.len(), |(p, _)| p);

    // ... more code goes here ...
}

But looking through my code, I tend to have a leading empty line whenever a block of code is split up in multiple segments. Otherwise I find the signature and the first part bleed together. I do skip that when there is a single { at the previous line, e.g. when a long where clause happened.

@solson
Copy link
Member

solson commented Mar 28, 2017

Ah, I misunderstood your previous comment. I would definitely omit that blank line in my own code.

@joshtriplett
Copy link
Member

@solson Likewise.

@nrc
Copy link
Member

nrc commented Apr 3, 2017

We discussed this at the meeting today. The basic framework is that Rustfmt should be configurable to specify the minimum and maximum number of line breaks allowed for both statements and items. The default minimum would be 1 and maximum would be 2. I.e., you can have either 0 or 1 blank lines between everything.

An extension would be to have min and max options for one-line items. This would address @solson's use case. By default these would have the same value as the multi-liners. However, by using 0 here and 1 for multi-line items (both min and max) you get totally locked down formatting. @solson, does this satisfy you?

We agreed that at least initially it is best to have some flexibility, rather than trying to nail down exact rules for when to allow blank lines. In the long term we could add more rules, but I fear that the cost in implementation complexity (risk of getting it wrong in some cases) outweighs the benefit.

We decided against allowing blank lines to start/end blocks.

@solson
Copy link
Member

solson commented Apr 6, 2017

@nrc That sounds good to me.

@OtterCode
Copy link

I'd like to pitch in strongly against the idea of a single empty line between top-level code. Even with documentation, two empty lines between major sections of code is a huge help when scanning quickly through a file. I would suggest the following as a default:


/// Documentation
fn foo() {

    ...

}


/// Documentation
fn bar() {

    let baz = x;
    let qux = y;

    // comment
    baz * qux

}

There's no need for a cramped style that limits spacing. This is a compiled language, and space is cheap and helpful. I come from a publishing background, and large, significant margins are crucial to the quick comprehension of text, especially in technical lists. I would put forward that the default be two blank lines between function or module definitions, and a single leading and trailing blank line in function definitions, with permitted blank lines within the function body.

We want to encourage people to write large. This also helps people have an incentive to keep function bodies short and concise. Additionally, extra spacing can visually isolate unusually long one-liners, encouraging refactoring.

@steveklabnik
Copy link
Member

steveklabnik commented Apr 27, 2017 via email

@nrc
Copy link
Member

nrc commented May 1, 2017

@OtterCode I think the consensus is against you, sorry. However, this definitely seems the sort of thing that reasonable people can disagree about and thus should be supported by configuration options, so although your proposal will not be the default, you will be able to configure Rustfmt to do what you want.

@LukasKalbertodt
Copy link
Member

Sorry for commenting on this old issue, but I strongly disagree with the reasoning for limiting the number of empty lines that users can put between statements. I commented here, but here is a copy for convenience:

May I ask why the default is 1? That's basically the only thing where I disagree with the standard style. I'd love to avoid having a rustfmt.toml.

My reasoning: more space between items is a useful visual separator. Rust files tend to get fairly long and often contain many different kinds of items. Compare that to some other languages, where there is usually just one class per file, for example. So in Rust files I often have "blocks" of items that somehow belong together. Maybe it's something like:

struct A;

impl A {
}

impl Display for A {
}


struct B;

impl B {
}

impl Display for B {
}

I think it should be pretty clear that some items belong closer together than others (specifically: all the A things should be somehow separated from all B things). And this is just a small example. Often I have multiple "layers" of items. So I naturally want to have larger separation between items of different "blocks". I usually use up to three blank lines between items.

So I'd like to propose increasing the default value to 3 or even 5. I think rustfmt shouldn't really be getting in the way of the programmer in this case.

This is basically the argument mentioned by several different people in this thread already. I do understand that some people might prefer limiting the empty lines. However, it seems wrong to me to enforce this style choice so aggressively. I think enforcing style makes sense where a divergence from that style seriously hurts readability. I don't think that's the case here.

So I'd really like to get this upper limited removed or at least relaxed to some fairly high number. From reading this thread, it also doesn't seem to me at all that there is a clear consensus. Several people argued in favor of using multiple empty lines as visual separator or to at least not enforce this behavior from the official Rust style/rustfmt.

@joshtriplett
Copy link
Member

First of all, I certainly think it's reasonable for rustfmt to allow configuring this.

This issue, however, is about the default style. And in the default style, I personally believe we should continue eliminating excess blank lines, just as we eliminate excess horizontal space between tokens. The default style exists to provide consistency, formatting the same code the same way for everyone.

Personally, if I feel that a single blank line doesn't separate code enough, I tend to add a comment rather than an extra blank line.

@LukasKalbertodt
Copy link
Member

First of all, I certainly think it's reasonable for rustfmt to allow configuring this.

Absolutely. I think we all agree on that. This already exists as an option, but it's nightly only. To stabilize it, we need to decide whether the default value is good -- which I don't think is the case.

eliminating excess blank lines, just as we eliminate excess horizontal space between tokens

I don't think this is a good comparison. Lines are limited in length already, thus a larger separation between "less related" parts of the line is not as important. Rust files on the other hand regularly have over 1000s of lines.

The default style exists to provide consistency, formatting the same code the same way for everyone.

And that's good. I really like having a more or less community-accepted style in Rust. Of course, such a default style has to strike a fine balance between permissive and restrictive. It wouldn't make sense to put a rule in the default style that a large majority of the community disagrees with, right? That would only lead a large part of the community to abandon the default style alltogether.


So how does the community think? I checked from this and the tracking issue. See the full details:

(I tried really hard not to include all comments or upvotes/downvotes. If I missed something, please tell me.)

In favor of only (0 or) 1 lines:

In favor of allowing more than 1 empty line:

(And in case that matters, three of those comments mentioned "strong disagreement")

nrc commented in this thread a lot, but I think they stated a definitive opinion on what the default should be. They commented this, however:

[...] People often use newline semantically - no lines between related statements, a blank line between separate groups of statements, multiple blank lines to enforce an even greater separation. [...] Furthermore, it is really easy to fix manually - you just delete them, its not like you have to re-lay out code.

(This matches my experience. I also saw many people using newlines like that.)

When collecting and deduping all people that commented, upvoted or downvoted, the result is:

0 or 1 lines

  • anderejd
  • gsingh93
  • joshtriplett
  • KalitaAlexey
  • pyfish
  • solson
  • steveklabnik
  • stjepang
  • tshepang

Do not restrict to 1 line

  • chriskrycho
  • cztomsik
  • felixc
  • ForLoveOfCats
  • gMatas
  • hcpl
  • killercup
  • kvark
  • LukasKalbertodt
  • OtterCode
  • phaylon
  • samvdris
  • swarmer

In total: 9 people in favor of restricting to 0 or 1 empty lines, 13 people against this restriction.

Obviously, that's a really small sample size and does probably not exactly represent the whole Rust community. However, it is enough (in my opinion) to question the current default value. Additionally, I think that a restriction in the standard style should require a larger majority than to keep it permissive.

What do you think about creating a PR changing the current default, and asking the community (via the usual means) to upvote or downvote that PR to make their opinion heard? If not, how do we resolve this?

@solson
Copy link
Member

solson commented Apr 15, 2020

Additionally, I think that a restriction in the standard style should require a larger majority than to keep it permissive.

I disagree, I think it's more valuable to have a consistent default style (even if I disagree with it in parts) than to have a loose style that allows code to be more inconsistent. I think a much stronger argument would need to be made for the style to be permissive by default than to have a specific restriction that some people dislike. For the most part we want rustfmt to act as a normalizer by default, bringing code with lots of little differences into a recognizable normal form, which we have to consider every time we allow multiple styles.

On another note, we really can't make decisions based on upvotes and downvotes - it's essentially always a small, unrepresentative sample of the community. The default style can never be perfect for all people with strong style opinions, anyway, but I hope we can convince most people that community-wide consistency is more valuable than personally tailored preference, and at worst a short custom rustfmt configuration should do the trick.

@LukasKalbertodt
Copy link
Member

Thanks for your comments. While I still think this particular default is really not great, I see your point. I guess we can then go ahead and stabilize this rustfmt option.

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

No branches or pull requests