rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.76k stars 2.42k forks source link

CI requires formatting, but the project doesn't specify which one #14442

Closed kornelski closed 2 weeks ago

kornelski commented 2 months ago

Problem

I have a custom system-wide rustfmt configuration, which doesn't match what Cargo wants, and running cargo fmt on my machine causes a complete mess.

Please add a rustfmt.toml to the project to nail down the formatting required.

epage commented 2 months ago

As a maintainer of projects more generally, I don't want to add empty files to my projects (there are already enough random files). I'm surprised rustfmt supports this and think having a non-default settings is setting yourself up for trouble.

Others on the team may disagree, there might be reasons specifically for cargo to do this, or there might be a reason to change my mind. Open to hearing more thoughts.

weihanglo commented 2 months ago

I don't object to adding a file. Have seen contributors tweaking unrelated formatting in pull requests, I should have realized it might be a global rustfmt.toml doing that.

It makes the contributing process better, so I am in.

kornelski commented 2 months ago

The global rustfmt config is painful indeed. But it's easier to add empty config files to reset to defaults, than to have unchanged defaults and keep copying and maintaining copies of big non-default config files everywhere else.

The only problem with resetting to default, is that Rustfmt is not entirely idempotent/input-independent (e.g. wraps comments and that can't be undone). This makes it impossible to undo wrong accidental formatting by reformatting with the correct settings. A single mistake can ruin the work in progress, and it'd be better to prevent that reliably.

epage commented 2 months ago

The global rustfmt config is painful indeed.

Could you help me understand your use case for why you do it?

kornelski commented 2 months ago

For my day job, I'm writing domain-specific code that frequently uses certain code patterns that rustfmt doesn't have any dedicated rules for, and basically doesn't understand they exist. It applies its general heuristics, which in my case are a poor fit, and too often result in code that is significantly harder to follow and harder to maintain.

However, because "readability" is assumed to be subjective to the point it's impossible for anybody to state that something is objectively worse, and any disagreement with a universal style guide is suspected to be merely provoking an unproductive bikeshedding about unimportant details, I'm unable to convince anybody that there is a problem with rustfmt that needs fixing. People tell me to stop complaining, and just configure rustfmt the way I like it, so I did. Rustfmt doesn't have options to avoid ruining the specific code patterns that I care about, so I'm forced to tweak multiple other global settings to minimize rustfmt's disruptions.

epage commented 2 months ago

In hopes to better undetstand what you are saying, is $DAYJOB that pushes back against changing their rustfmt config or the rustfmt team doesn't accept input?

If $DAYJOB needs some specific formatting, it seems like it should live $DAYJOB's repo and not on each users system.

kornelski commented 2 months ago
  1. I work on many more projects where I need my custom formatting than projects that reject PRs over formatting differences.

  2. A custom config file is harder to maintain across many repos than the default config. I can't symlink to my ~/.rustfmt.toml, because that will work only on my machine. I'd rather not sync many copies across many projects, and I'd prefer cargo new to use my config.

  3. The global config in rustfmt exists, and it conceptually clashes with an implied-default fmt --check in CI. I think in rustfmt as it exists today, the correct solution is to define the config to check against.

I've opened an issue about this dilemma.

The notion that formatting should be automated creates manual work for me either way, and that is really frustrating.

epage commented 2 months ago

Thats now being tracked at rust-lang/rustfmt#6264

epage commented 2 months ago

A custom config file is harder to maintain across many repos than the default config. I can't symlink to my ~/.rustfmt.toml, because that will work only on my machine. I'd rather not sync many copies across many projects, and I'd prefer cargo new to use my config.

If the formatting is common between them, then there are likely other aspects in common between them that people need to keep in-sync. For myself, I keep https://github.com/epage/_rust as a merge-base (initially added via --allow-unrelated-histories) to keep CI workflows, lint config, etc common between all of my projects in an easy-to-update fashion. Having a git-synced file shared between the projects seems like a much better on-boarding experience rather than asking everyone touching those code bases to have a user config.

The global config in rustfmt exists, and it conceptually clashes with an implied-default fmt --check in CI. I think in rustfmt as it exists today, the correct solution is to define the config to check against.

I feel this is patching over the problem. This is further cementing user configs without helping the user not apply it when it isn't wanted, leaving the user to still wait until CI fails before they find out what happens. If we define a config to check, then we'd have a blank file somewhere anyways so that seems to be shuffling things around.

ehuss commented 2 weeks ago

The cargo team discussed this issue and have decided to close it as something we would prefer to not add at this time. In general we felt like the approach of using a global config is not a pattern we want to encourage. We understand that it is a problem dealing with a large config across other projects. I might suggest placing projects in subdirectories with a parent rustfmt.toml file that contains whether settings is appropriate for those projects.