Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

use rustfmt #1668

Closed
dvc94ch opened this issue Jul 18, 2020 · 9 comments
Closed

use rustfmt #1668

dvc94ch opened this issue Jul 18, 2020 · 9 comments

Comments

@dvc94ch
Copy link
Contributor

dvc94ch commented Jul 18, 2020

Why use rustfmt? The main reason is so you don't annoy developers when they accidentally run it before committing their changes.

@tcharding
Copy link
Contributor

Yes please, it is a real drag to have to turn off format on save in your editor everytime one wants to work on rust-libp2p when ones daily work is on a rust code base that uses rustfmt. It also means I can't have a buffer open doing rust-libp2p work and a buffer open to other repos without have to reset all the formatting changes, while manageable its a drag to have to think about this stuff.

Can we configure rustfmt in a manner that is acceptable to the rust-libp2p project?

@burdges
Copy link

burdges commented Jul 20, 2020

Rust was designed to be expressive, which makes Rust far harder to fmt than languages explicitly designed to be inexpressive, and so rustfmt ends up being fairly crappy.

We could set rustfmt.toml to ignore = ["/"] according to https://github.com/rust-lang/rustfmt/blob/master/Configurations.md except that never actually worked: rust-lang/rustfmt#4147 (comment)

As luck would have it, we could exploit rustfmt's bugginess for this by simply doing https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=856d116c793b74e6809441b4ccf7af68 in lib.rs or somewhere so that rustfmt panics rust-lang/rustfmt#3678

@rklaehn
Copy link
Contributor

rklaehn commented Jul 23, 2020

@burdges out of curiosity: could you point out examples where the rustfmt formatted code is so bad? I guess the huge lists of type constraints on some parts of the code will look bad. Maybe these sections can be manually excluded. In general I think fmt does an OK job.

@burdges
Copy link

burdges commented Jul 23, 2020

I only know my own code, not the libp2p crate, but..

It screws up any whitespace that communicates emphasis, especially with iterator chains like https://github.com/w3f/schnorrkel/blob/master/src/vrf.rs#L944 so you need far more temporaries, which becomes excessive. There are many situations even post-NLL where Rust requires closures be handled in line, not as temporaries.

Anyways Parity has their own style guide and afaik they ban rustfmt from their code bases. I hope this becomes more commonplace..

@dvc94ch
Copy link
Contributor Author

dvc94ch commented Jul 23, 2020

Actually there are parity codebases that use rustfmt. Gav doesn't like it so it's not used in substrate or polkadot, but many smaller crates/repos it's up to the devs basically. Also note that this is a libp2p repo not a paritytech repo.

@dvc94ch
Copy link
Contributor Author

dvc94ch commented Jul 23, 2020

While my preference is for consistent formatting that doesn't require devs to learn a new style guide for each repo they contribute to, finding a way for rustfmt to simply ignore and not do anything is an acceptable solution for me. Note that code style is in many ways subjective and on a large project with many devs there is going to be inconsistent formatting even with a style guide.

@rklaehn
Copy link
Contributor

rklaehn commented Jul 23, 2020

I think in 95% of the time rustfmt does an OK job, and for the remaining 5% I am fine with manually disabling it.

But if that is not an option, just disabling it for the entire crate would already be an improvement for me.

@tcharding
Copy link
Contributor

Any solution that doesn't require me to change my editor config to work on the repository is fine for me. Consistency is good, the granularity doesn't matter much I'd say i.e., more than one style in a repo but consistent within xyz modules - is libp2p really big enough or old enough to warrant that?

@JayKickliter
Copy link

Why use rustfmt? The main reason is so you don't annoy developers when they accidentally run it before committing their changes.

Yes and pretty-please. It's going to make a pretty big diff the first time, which no one likes. But the longer we wait, the worse that diff is going to be. But it's still worth the initial diff noise. It's helpful for maintainers and reviewers in that they don't have to bother with asking formatting changes on PRs. It helps first time contributors (I'm feeling this right now) by not having to guess the projects style.

It screws up any whitespace that communicates emphasis, especially with iterator chains like

I feel you, and empathize. In the cases where it legitimately makes for illegible code, locally disabling is an option. But I haven't seen rustfmt do that since maybe the first few releases.

@romanb romanb closed this as completed Feb 23, 2021
@libp2p libp2p locked and limited conversation to collaborators Feb 23, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants