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

[unstable option] blank_lines_upper_bound #3381

Open
scampi opened this issue Feb 13, 2019 · 14 comments
Open

[unstable option] blank_lines_upper_bound #3381

scampi opened this issue Feb 13, 2019 · 14 comments
Labels
unstable option tracking issue of an unstable option

Comments

@scampi
Copy link
Contributor

scampi commented Feb 13, 2019

Tracking issue for unstable option: blank_lines_upper_bound

@scampi scampi added the unstable option tracking issue of an unstable option label Feb 13, 2019
@scampi scampi changed the title [unstable option] unstable option: blank_lines_upper_bound [unstable option] blank_lines_upper_bound Feb 18, 2019
@LukasKalbertodt
Copy link
Member

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.

But maybe people have strong opinions against large separators. I tried searching for previous discussions about this, but I found nothing. So please let me know either way!

@ForLoveOfCats
Copy link

I would also like to voice my support for stabilizing this option. I personally use single lines between "paragraphs" in blocks of code, two lines between related functions and structs, and three lines between unrelated sections of source. This feature requiring nightly is rather frustrating

@cztomsik
Copy link

why is it nightly? I think it makes sense to be able to set it to 2

@LukasKalbertodt
Copy link
Member

The discussion here seems like we will not change the default, unfortunately. So we could go ahead and stabilize this option. Only these conditions left:

  • The design and implementation of the option are sound and clean.
  • The option is well tested, both in unit tests and, optimally, in real usage.
  • There is no open bug about the option that prevents its use.

@akhouderchah
Copy link

The discussion here seems like we will not change the default, unfortunately. So we could go ahead and stabilize this option. Only these conditions left:

  • The design and implementation of the option are sound and clean.
  • The option is well tested, both in unit tests and, optimally, in real usage.
  • There is no open bug about the option that prevents its use.

Where does this leave us now? Doesn't look like there are bugs open about the option other than this.

The other two points seem a bit subjective; a rust n00b like me certainly can't weigh in there, but I agree with the others that having this option in stable would be very nice.

@calebcartwright
Copy link
Member

As a reminder, issues for tracking stabilization of rustfmt configuration options are not intended to be used for debating default formatting behavior set by the Style Guide that governs rustfmt's default behavior. Additionally, the discussion around this particular aspect of the default behavior in the Style Guide is closed, so additional comments about the default, particularly here within the rustfmt repository, are not particularly productive.

As far as actually stabilizing a configuration option, the process and requirements are defined here. While I appreciate that folks are eager to have the option available on stable, that really isn't the driver/determination of stabilization. Rust takes stability very seriously, and the formatting guarantee for rustfmt imposes additional constraints on the development of rustfmt and stabilization of features.

We won't stabilize any option unless and until those criteria have all been easily cleared and we're sufficiently confident in the implementation and that it won't cause any buggy or unexpected formatting behavior anywhere, regardless of how long the option has existed in unstable or how many users are interested in stabilization. I know it can be frustrating/annoying for options to not be available on stable, but premature stabilization is absolutely unacceptable and why we always err on the side of caution and taking longer.

blank_lines_upper_bound, much like blank_lines_lower_bound, hasn't hit the threshold for stabilization yet, both because there's some known issues/gaps and because the current implementation in the released versions of rustfmt is likely not the direction we want to go (#4295, #2954, #4082, etc. note that some closed issues are still applicable/blocking)

@bluenote10
Copy link
Contributor

The option is well tested, both in unit tests and, optimally, in real usage.

I'm not sure how real usage is quantified typically, but for what it's worth, the feature is working well for us in a number of projects, and it is the main reason why these projects rely on the 2.0 RC branch.

felixc added a commit to felixc/rexiv2 that referenced this issue Aug 3, 2022
For now, this has to use the nightly version of Cargo/rustfmt, because I
really want to preserve the multiple newlines between sections of code,
and that requires the `blank_lines_upper_bound` configuration option in
`.rustfmt.toml`. See rust-lang/rustfmt#3381
forthe bug tracking the stabilization of this configuration option.
felixc added a commit to felixc/rexiv2 that referenced this issue Aug 3, 2022
For now, this has to use the nightly version of Cargo/rustfmt, because I
really want to preserve the multiple newlines between sections of code,
and that requires the `blank_lines_upper_bound` configuration option in
`.rustfmt.toml`. See rust-lang/rustfmt#3381
for the bug tracking the stabilization of this configuration option.
@TobTobXX
Copy link

TobTobXX commented Sep 2, 2022

Only these conditions left:

  • The design and implementation of the option are sound and clean.

Can't comment on that.

  • The option is well tested, both in unit tests and, optimally, in real usage.

I don't know about being well tested in unit tests, but real world usage is definitely present. Search for blank_lines_upper_bound on GH and file type TOML -> ~1600 results, which is absolutely comparable to the stable remove_nested_parens (~2500 results), even when disregarding the hurdle to use unstable features (ie. the nightly chain)

  • There is no open bug about the option that prevents its use.

The only one that's referenced is #4082, which appears to be a wontfix.

@dxps
Copy link

dxps commented Dec 19, 2022

Dear all,

I would really like to understand which option would allow me to have an explicit blank line between a fn signature and its body ❓ Asking since initially I hoped this option would allow it.

I really want this (and used it in other languages) for the cases where the function signature is long or complex enough to need a blank line between it and its body to improve readability.

For example:

  • Instead of having such formatted result:
      pub async fn add(
          &self,
          slug: &String,
          title: &String,
          description: &String,
          body: &String,
          tag_list: &Vec<String>,
          author_id: i64,
      ) -> Result<DateTime<Utc>, AppError> {
          let mut txn = self.dbcp.begin().await?;
          let article_id: i64;
          let created_at: DateTime<Utc>;
  • I'd really prefer a clear separation (for a better readability):
      pub async fn add(
          &self,
          slug: &String,
          title: &String,
          description: &String,
          body: &String,
          tag_list: &Vec<String>,
          author_id: i64,
      ) -> Result<DateTime<Utc>, AppError> {
          
          let mut txn = self.dbcp.begin().await?;
          let article_id: i64;
          let created_at: DateTime<Utc>;

Testing using the nightly toolchain (by running rustup override set nightly, plus even creating a rust-toolchain file with the content of nightly within it, part of a cargo new ... project), having:

fn main() {

    println!("Hello, world!");
}

and running rustfmt --unstable-features src/main.rs it formats it as:

fn main() {
    println!("Hello, world!");
}

Thanks.

@marcospb19
Copy link

marcospb19 commented Dec 19, 2022

@dxps I don't think that's possible.

EDIT: for reference, [clang-format calls this KeepEmptyLinesAtTheStartOfBlocks].

EDIT2: I deleted my previous comment arguing that the default should be changed, after getting used to 1, I think it does not matter.

@dxps
Copy link

dxps commented Dec 19, 2022

@marcospb19 Appreciate the quick feedback! 🙏
Sad, but true, some people are enforcing some things ... 😞
Iirc, that was the case with Dart as well.

@tkr-sh
Copy link

tkr-sh commented Feb 6, 2024

It's there any plans on passing that as stable ?

@acanton77
Copy link

What is so difficult in making this option stable? It has been years.

@calebcartwright
Copy link
Member

It's there any plans on passing that as stable ?

What is so difficult in making this option stable? It has been years.

please see #5365 and #5367

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

Successfully merging a pull request may close this issue.