Skip to content

Add blankline_before_or_after_brace option. #5173

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

Open
your-diary opened this issue Jan 15, 2022 · 10 comments
Open

Add blankline_before_or_after_brace option. #5173

your-diary opened this issue Jan 15, 2022 · 10 comments

Comments

@your-diary
Copy link

your-diary commented Jan 15, 2022

Environment: rustfmt 1.4.37-stable (f1edd042 2021-11-29) (with no configuration file)

Currently, blank lines before the opening brace { or after the closing brace } are removed.

fn main() {

    println!("Hello, world!");

}

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

This style is hard to read especially when the signature (plus where clause) and the body are long:

use regex::Regex;
use std::process::Command;
use std::process::Output;

fn _some_function<T, U, V, W>(
    _some_param_1: T,
    _some_param_2: &U,
    _some_param_3: Vec<V>,
    _some_param_4: &[W],
    _some_param_5: String,
) -> String
where
    T: Iterator,
    T::Item: Clone,
    U: std::fmt::Display + Sync,
    V: std::hash::Hash,
    W: PartialEq + PartialOrd,
{
    let regex = Regex::new(r"\$\((.*)\)").unwrap(); // <--- I want a blank line above this line!

    let mut s = String::new();

    match regex.captures(&s) {
        Some(c) => {
            let command = c.get(1).unwrap().as_str();
            let range = c.get(0).unwrap().range();
            let res: Output = Command::new("bash").args(["-c", command]).output().unwrap();
            let res: String = String::from_utf8(res.stdout).unwrap().trim().to_string();
            s.replace_range(range, &res);
        }
        None => (),
    }

    let a = 5;
    println!("{}", a);

    std::env::args().for_each(|e| println!("{}", e));

    String::from("hello")
}

I've checked all the stable and unstable configuration options listed in Configuring Rustfmt, but it seemed there was no option to preserve blank lines before or after braces.

So this is a feature request:

blankline_before_or_after_brace

Default value: false

Possible values: false, true, preserve

Behavior:

  • false (default): No blank line before or after braces is allowed.

  • true: Always insert a blank line before or after each brace.

  • preserve: Preserve the style in the original code.

@calebcartwright
Copy link
Member

Realize some old branching decisions have complicated the ability to parse the issue tracker, but this is largely duplicative of #2868 and implementation in #4303.

Hopefully most, or at least a good chunk of #4303 could be backported as a starting point though I think it'd be worthwhile to revisit the implementation in light of the ask above/question I surfaced in #4303 (comment)

@your-diary
Copy link
Author

Thank you for you response. I've read the three links you gave plus #4438 , but I don't fully tell the time series and the current status.

My understanding of the time series is

  1. The option did once exist, and its name was remove_blank_lines_at_start_or_end_of_block. (#2868 (comment))

  2. This option was removed at some time. (#2868 (comment))

  3. Configuration option to preserve blank lines at beginning of block #2868 was issued to revert the removal of the option.

  4. This revert (but only with around opening brace and renamed to preserve_block_start_blank_lines?) was finally implemented and merged. (Configuration option to preserve blank lines at start of block #4303)

  5. preserve_block_start_blank_lines config simply doesn't exist #4438 was issued which says preserve_block_start_blank_lines is simply unavailable (though merged in Configuration option to preserve blank lines at start of block #4303).

Now I'm very confused. In #4438, someone says Closing since I discovered that apparently, it's for v2 but not v1.4. and a screenshot of the documentation which refers to the option is posted. However, if I now check the documentation of v2.0.0-rc2 (when I first issued this issue, I only checked master version), preserve_block_start_blank_lines doesn't exist.

Maybe I misunderstand something...?

@calebcartwright
Copy link
Member

calebcartwright commented Jan 16, 2022

Maybe I misunderstand something...?

Yes, your understanding is off, but my apologies if I've caused any confusion.

tl;dr - Your feature request was (partially) implemented, but it was implemented on a different branch that was for an experimental version of rustfmt which was never released, and which will not be released any time soon (if ever). As such, the feature still needs to be implemented on the main/official rustfmt version, but I've tagged in details from the experimental work because it should serve as a useful reference, and potentially even has some work that can be directly utilized by cherry-picking commits from the other branch

@murchu27
Copy link
Contributor

@calebcartwright can you assign this to me? Will try cherry-picking, and if I have any issues I'll reach out

@calebcartwright
Copy link
Member

@calebcartwright can you assign this to me? Will try cherry-picking, and if I have any issues I'll reach out

We've got triagebot wired up, can you try adding a comment with @rustbot claim?

@calebcartwright
Copy link
Member

Also, thanks for your interest @murchu27! We don't exactly have contributors racing each other with competing PRs so you should be all clear to work on this one 😀

We do need to figure some things out relative to the solution prior to grabbing any commits though. Would encourage you to take a look at some of the dialog/questions/considerations and share any thoughts or proposals you may have.

@murchu27
Copy link
Contributor

@rustbot claim

@murchu27
Copy link
Contributor

Nice one, and yes I'll carve out some time over the weekend to read more into this

@ppamorim
Copy link

ppamorim commented Feb 18, 2022

Hi @murchu27, is there any way to implement that as a custom option? My code is getting very convoluted.

@murchu27
Copy link
Contributor

Hey @ppamorim , by "custom option" do you mean "config option"? If you're asking whether I've made progress on this, then unfortunately not!

It's still very much on my task list, but happy to hand this over to someone else if there's a taker. I know @ayazhafiz worked on this before, but I'm not sure if they have time to contribute right now

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

No branches or pull requests

5 participants