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] brace_style #3376

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

[unstable option] brace_style #3376

scampi opened this issue Feb 13, 2019 · 22 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: brace_style

@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: brace_style [unstable option] brace_style Feb 18, 2019
@misos1
Copy link

misos1 commented Dec 13, 2019

Seems brace_style = "AlwaysNextLine" does not work on unsafe blocks.

@Sibz
Copy link

Sibz commented Sep 9, 2020

Seems brace_style = "AlwaysNextLine" does not work on logic blocks either.

@calebcartwright
Copy link
Member

@Sibz could you elaborate on what you mean by logic blocks, with a code snippet? If you're referring to things like if, else etc. then you'll want to check out the control_brace_style option https://rust-lang.github.io/rustfmt/?version=master&search=#control_brace_style

@Sibz
Copy link

Sibz commented Sep 15, 2020

Thank you, didn't see that one and I did scour the whole list! Mustn't have had my morning coffee.

@Sibz
Copy link

Sibz commented Sep 30, 2020

New structs aren't included in this
i.e.

let value = MyStruct {
    something: 1
}

vs

let value = MyStruct
{
    something: 1
}

@Iron-E
Copy link

Iron-E commented Aug 4, 2021

Closures are also not included in this.

foo.ok_or_else(|| {
  // closure
})

Should be:

foo.ok_or_else(|| 
{
  // closure
})

@Iron-E
Copy link

Iron-E commented Aug 5, 2021

Sorry for double comment. Also does not work for macro_rules!:

macro_rules! filter_map_view {
  ($query:ident, $val:ident) => {

Should be:

macro_rules! filter_map_view 
{
  ($query:ident, $val:ident) => 
  {

@benaryorg
Copy link

Also doesn't work for use.

Input

Using brace_style = "AlwaysNextLine":

use ::
{
	std::time::Duration,
	async_std::
	{
		fs::remove_file,
		path::Path,
		task,
	},
	clap::
	{
		app_from_crate,
		crate_authors,
		crate_description,
		crate_name,
		crate_version,
		Arg,
	},
	log::
	{
		error,
		info,
	},
};

Expected

No changes.

Actual Result

use ::{
	async_std::{
		fs::remove_file,
		path::Path,
		task,
	},
	clap::{
		app_from_crate,
		crate_authors,
		crate_description,
		crate_name,
		crate_version,
		Arg,
	},
	log::{
		error,
		info,
	},
	std::time::Duration,
};

@calebcartwright
Copy link
Member

At this point I think it's worth pausing to reconsider whether the current option is the right approach to meet the needs of users. For reference, this option was historically only applied to items; not in the expression nor statement contexts. control_brace_style also exists to configure the brace style on control flow type expressions.

We'd started extending the option to cover a few additional expression contexts, such as on unsafe, async, and extern blocks. However, I'm increasingly getting the sense that having only one option (essentially) is unlikely to provide a sufficient amount of flexibility. I wonder if instead we might need to a tiered approach that provide more options with more granular control, as well as a top level "big hammer" option that cascades everywhere unless overridden by a more granular one (similar to the width heuristic options). For example, while some folks may want every brace to always be on its own line everywhere, others may indeed just want the style to apply to items and not say the bodies of their multiline match arms.

This option as originally intended and implemented, controlling brace style for items, is actually probably a fair candidate for stabilization at this point, but if we need to extend it out to cover every expression context as well then it's likely to remain unstable for years to come.

@Iron-E
Copy link

Iron-E commented Aug 28, 2021

I think some of the confusion comes from the qualified naming of control_brace_style and unqualified naming of brace_style. You might consider renaming this option to item_brace_style if that is the intention, and move unsafe, async, and extern support to a new brace_style.

@misos1
Copy link

misos1 commented Sep 5, 2021

I wonder if instead we might need to a tiered approach that provide more options with more granular control, as well as a top level "big hammer" option that cascades everywhere unless overridden by a more granular one

That sounds great.

This option as originally intended and implemented, controlling brace style for items, is actually probably a fair candidate for stabilization at this point

I hope this does not mean that it cannot be renamed.

@calebcartwright
Copy link
Member

I hope this does not mean that it cannot be renamed.

Guess I'd missed this, but no, it has no impact. Even stabilized options can be renamed, provided we can do so without running into name conflicts with other options, as we can do such renames in a non-breaking manner.

Obviously unstable options can be renamed or even removed as necessary.

@milewski
Copy link

milewski commented Sep 29, 2023

It would also be good to have a bit more specificity, for instance, be able to apply it only to certain types of elements and ignore others, for example:

Good

pub struct Elevator { // Would be nice to have a way to ignore it applying to structs 
    projector: Projector,
    height_map: GrayImage,
}

impl Elevator
{
    pub fn new(radius: f64, origin: Coordinate, height_map: GrayImage) -> Self
    {
        Self {
            height_map,
            projector: Projector::new(radius, origin),
        }
    }
}

Bad

pub struct Elevator
{
    projector: Projector,
    height_map: GrayImage,
}

impl Elevator
{
    pub fn new(radius: f64, origin: Coordinate, height_map: GrayImage) -> Self
    {
        Self {
            height_map,
            projector: Projector::new(radius, origin),
        }
    }
}

@SnipeHub
Copy link

SnipeHub commented Feb 4, 2024

Is there a reason why RUST put's it's open bracket on the same line. Is it a performance thing? It's definitely not easier to read. Most languages place it on a new line, why change what works? I prefer the old method, unless someone knows why we shouldn't.

@misos1
Copy link

misos1 commented Feb 4, 2024

Is there a reason why RUST put's it's open bracket on the same line. Is it a performance thing? It's definitely not easier to read. Most languages place it on a new line, why change what works? I prefer the old method, unless someone knows why we shouldn't.

It is just a matter of personal taste. Also I prefer it on a new line. Just seems the author of rustfmt probably prefers it differently. 4 years passed and this is still not resolved, still no progress? Other tools like for example linters even for javascript have options, but not rustfmt, this is what holds me back from using this otherwise seemingly great tool (for 4 years now).

@SnipeHub
Copy link

SnipeHub commented Feb 4, 2024

Is there a reason why RUST put's it's open bracket on the same line. Is it a performance thing? It's definitely not easier to read. Most languages place it on a new line, why change what works? I prefer the old method, unless someone knows why we shouldn't.

It is just a matter of personal taste. Also I prefer it on a new line. Just seems the author of rustfmt probably prefers it differently. 4 years passed and this is still not resolved, still no progress? Other tools like for example linters even for javascript have options, but not rustfmt, this is what holds me back from using this otherwise seemingly great tool (for 4 years now).

It appears it's probably not a very important feature for them if it's remained unstable for 4 years. I guess I'll just have to press the space bar one extra time, not a big deal. I'll pass on this until they decide to fix it.

@ytmimi
Copy link
Contributor

ytmimi commented Feb 8, 2024

Hey all 👋🏼, just wanted to chime into the discussion with a little bit of info.

Is there a reason why RUST put's it's open bracket on the same line. Is it a performance thing? It's definitely not easier to read. Most languages place it on a new line, why change what works? I prefer the old method, unless someone knows why we shouldn't.

The default choice isn't based on any one persons opinion. rustfmt is primarily an implementation of the Rust Style Guide and the default formatting adheres to the guide. A lot of discussion has gone into it, and it's a living document that continues to evolve. These days, the guide is owned by the rust style team (check out the link for more details on the team).

@misos1
Copy link

misos1 commented Feb 8, 2024

The default choice isn't based on any one persons opinion. rustfmt is primarily an implementation of the Rust Style Guide and the default formatting adheres to the guide. A lot of discussion has gone into it, and it's a living document that continues to evolve. These days, the guide is owned by the rust style team (check out the link for more details on the team).

Ok so it is the opinion of some group of people. I think one of reasons is for crates to have consistent (the same) formatting and that is probably a good point. But this point should not apply to people using crates programming their projects, they can have their own style guides for their projects.

@ytmimi
Copy link
Contributor

ytmimi commented Feb 8, 2024

Ok so it is the opinion of some group of people

Yes and no. Style decisions are made after coming to a consensus. Anyone is free to open PRs to discuss new default styles. And just to be clear, the style guide only prescribes the default style. I was simply trying to answer the earlier question and clarify that the rustfmt team isn't making style related decisions.

@MassiminoilTrace
Copy link

Is there a reason why RUST put's it's open bracket on the same line. Is it a performance thing? It's definitely not easier to read. Most languages place it on a new line, why change what works? I prefer the old method, unless someone knows why we shouldn't.

It is just a matter of personal taste. Also I prefer it on a new line. Just seems the author of rustfmt probably prefers it differently. 4 years passed and this is still not resolved, still no progress? Other tools like for example linters even for javascript have options, but not rustfmt, this is what holds me back from using this otherwise seemingly great tool (for 4 years now).

It appears it's probably not a very important feature for them if it's remained unstable for 4 years. I guess I'll just have to press the space bar one extra time, not a big deal. I'll pass on this until they decide to fix it.

I'm in the same situation I guess. This is one of the most basic formatting features, and it's still in unstable despite it already works well. Currently I made a workaround in my projects, a small shell script that switches to nightly, formats code, then switches backs to stable toolchain.

How can I contribute to speed this up?
I'm not familiar with rustfmt code at all, but I have some middle-level rust experience.
What parts are currently missing to have full approval in stable?

@afetisov
Copy link

Currently I made a workaround in my projects, a small shell script that switches to nightly, formats code, then switches backs to stable toolchain.

I have no idea what you're talking about. Running a nightly rustfmt is as easy as cargo +nightly fmt, provided that you have installed a nightly toolchain and enabled unstable formatting features. It also doesn't affect the rest of your code in any way, you can easily stay on stable Rust while using unstable formatting options.

The only issue with unstable options is that they are not guaranteed to achieve Rust's quality standard, and not guaranteed to never change formatting between toolchain releases. Neither are likely to be an issue for this specific feature.

@ethanmsl
Copy link

ethanmsl commented Jun 1, 2024

brace_style = PreferSameLine || SameLineWhere
control_brace_style = AlwaysSameLine
do not work if the preceding element is multi-line (e.g. common when chain_width = 0)

Note 1: the .method in the preceding code means that the start of the branch code will not be confused with the multi-line element above it.
Note 2: I have Visual indenting on, but this is true regardless.

Is this intended? And if so may I suggest it not be or request a workaround?

Examples:

Current

Current_1

Desired

Desired_1

Current

Current_0

Desired

Desired_0

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

No branches or pull requests