feat: support formatting Cargo.toml

[xxchan]

trafficstars

From the disscussion in https://github.com/rust-lang/rustfmt/issues/4091, rustfmt should support format Cargo.toml since the Style Guide actually already includes a spec for Cargo.toml.

And things to consider include:

• formatting logic
• experience/usability perspective: (default behavior, cli args/opts, configurable via rustfmt.toml, etc)

As such I'd advise starting with the formatting logic (something that takes in an unformatted string and returns a formatted one) and we'll figure out the rest later on. Feel free to put this in a new mod under src/formatting

Originally posted by @calebcartwright in https://github.com/rust-lang/rustfmt/issues/4091#issuecomment-730716157

I do not think deferring the actual formatting logic to an external library is likely to be a viable candidate for the reasons that I've outlined above. We've got a formatting spec we'd have to follow, and even if one or more versions of said external lib happen to emit the same formatting, there's no guarantee they'd continue to do so (and we couldn't use a pinned version either due to risk of update blockage)

Originally posted by @calebcartwright in https://github.com/rust-lang/rustfmt/issues/4091#issuecomment-914846250

So in this PR I implemented the formatting logic according to the style guide. I used https://github.com/ordian/toml_edit to do so in an extensible way (visitors).

[calebcartwright]

Haven't looked at the diff yet but very excited to see this, thanks so much for getting the process started @xxchan!

[xxchan]

@calebcartwright @ytmimi Would you have some time recently to review this PR and give some feedbacks? At least for the general idea and code structure of the PR. 😊

[calebcartwright]

Thank you for your patience @xxchan. I'm currently in the closing steps of prepping the next release, and afterwards I'll circle back to this and some of the larger/older PRs that have been pending

[ytmimi]

I also want to acknowledge that I've seen this and say that I'll try to carve out some time next week to start a review

[xxchan]

@ytmimi Thanks for your review! I will resolve comments later.

For the calling path and tests, do we plan to keep working on this PR or get it merged first?

[ytmimi]

For the calling path and tests, do we plan to keep working on this PR or get it merged first?

I think I'd be fine either way, but it would definitely be ideal (at least in my opinion) if we could keep iterating on this PR before merging. I'll also wait for @calebcartwright to chime in before we decide how we'll move forward.

[calebcartwright]

For the calling path and tests, do we plan to keep working on this PR or get it merged first?

I think I'd be fine either way, but it would definitely be ideal (at least in my opinion) if we could keep iterating on this PR before merging. I'll also wait for @calebcartwright to chime in before we decide how we'll move forward.

Not sure if this answers any specifics, but generally speaking we can certainly batch the overall implementation across multiple PRs (smaller PRs are always better in my mind). However, let's also make sure that each chunk/PR is complete. We can certainly update signatures, move items around between files/mods, etc. but we should have whatever we add here fully tested and with the api we think makes sense at this point.

[ytmimi]

Hey @xxchan Thanks again for your work on getting this started. I know you mentioned that you were a little busy https://github.com/rust-lang/rustfmt/issues/4091#issuecomment-1083603647, so I went ahead and rebased your changes on the current master and add the additions to get format_cargo_toml on a path that will be called by rustfmt.

I probably want to add a few more tests, but I think we're in a good place implementation wise.

[gilescope]

Excellent can't wait for this to land so that we can stop manually worrying about how Cargo.toml should be formatted. This is the improvement in rust I am most looking forward to this year!

[calebcartwright]

Excellent can't wait for this to land so that we can stop manually worrying about how Cargo.toml should be formatted. This is the improvement in rust I am most looking forward to this year!

Just to be clear, this PR doesn't fully implement the feature in and of itself, just an important first step

[gilescope]

I’ll take a first step!

On Sun, 8 May 2022 at 19:23, Caleb Cartwright @.***> wrote:

Excellent can't wait for this to land so that we can stop manually worrying about how Cargo.toml should be formatted. This is the improvement in rust I am most looking forward to this year!

Just to be clear, this PR doesn't fully implement the feature in and of itself, just an important first step

— Reply to this email directly, view it on GitHub https://github.com/rust-lang/rustfmt/pull/5240#issuecomment-1120465190, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGEJCD5QRRVPUC6HQ7QQBLVJABDPANCNFSM5PLIHPZA . You are receiving this because you commented.Message ID: @.***>

[xxchan]

Anyone here can suggest more test cases like https://github.com/rust-lang/rustfmt/pull/5240/files#diff-3ab626d68c49333207c9209bf5abb01c7b2eff301470bd5dcc801da3dcf2345f? 😄

[xxchan]

I've added a new rule TrimSpaces, it's not described in style guide, but maybe it's expected

[xxchan]

Hi @ytmimi @calebcartwright, I added an unstable option format_cargo_toml and integrated the support for Cargo.toml to cargo-fmt, so I think this PR is now ready to be merged and let users play with it. 😊

[xxchan]

Thanks for your review!

I'm not sure if we're ready to add this to cargo fmt.

I believe most of users use cargo fmt instead of using rustfmt directly, and they expect cargo fmt to handle the whole workspace. So I think adding it to cargo fmt is a critical step for this long-awaited feature to have enough exposure and user testing. It's also the reason why I think this PR is good enough to be merged instead of being blocked for too long.

I will resolve other comments soon.

[xxchan]

Comments expect those about cargo-fmt are resolved.

[joshparallel]

Would love to use this feature! Is there something that we're waiting on?

[xxchan]

IMO it's good enough to merge. cc @ytmimi Could you confirm that and update the PR label?

[gilescope]

Great timing - this is top of my xmas list.

On Wed, 14 Dec 2022 at 08:29, xxchan @.***> wrote:

IMO it's good enough to merge. cc @ytmimi https://github.com/ytmimi Could you confirm that and update the PR label?

— Reply to this email directly, view it on GitHub https://github.com/rust-lang/rustfmt/pull/5240#issuecomment-1350625107, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGEJCATKIHOQELAQ7YG4VTWNGAOPANCNFSM5PLIHPZA . You are receiving this because you commented.Message ID: @.***>

[aurexav]

Please take some time on this. We really need this! It's been pending for a month.

[andrewgazelka]

Adding to the positive sentiment. This would be really useful for my team and me if this is merged soon! :)

[gilescope]

As soon as it’s in nightly then people can give lots of feedback - even if it’s behind a flag and not default for now.

On Mon, 6 Feb 2023 at 20:17, Andrew Gazelka @.***> wrote:

Adding to the positive sentiment. This would be really useful for my team and me if this is merged soon! :)

— Reply to this email directly, view it on GitHub https://github.com/rust-lang/rustfmt/pull/5240#issuecomment-1419693155, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGEJCAETUYSSAORYG7LY6LWWFL6BANCNFSM5PLIHPZA . You are receiving this because you commented.Message ID: @.***>

[iajoiner]

Are we going to merge this?? We are waiting for it.

[evertedsphere]

Adding to the chorus here: is there something holding this back?

[calebcartwright]

I'd kindly request that folks only comment here if they have feedback salient to the specifics of the proposed implementation. I understand the desire for the feature, but the repeated flavors of "+1" comments don't impact the priority+work vs. bandwidth challenges that we (and frankly most dev tools teams) have, and as such aren't going to result in anything happening more quickly.

These comments also make things a bit more challenging by extending the length of the thread and unnecessarily notifying those that have subscribed in order to be made aware of any substantial updates/movement.

[xxchan]

If you want to try this on your project:

git clone https://github.com/rust-lang/rustfmt && cd rustfmt
gh pr checkout 5240
cargo build --release --bin rustfmt --bin cargo-fmt -Zunstable-options --out-dir "$(rustc --print=sysroot)/bin"

This installation is safe and won't pollute other installations.

Then you will be able to use it as cargo +nightly-2023-01-24 fmt on any project. Note you will also need to add format_cargo_toml = true in rustfmt.toml.

[nyurik]

I have made a draft PR to a PR to try to resolve the current merge conflict - https://github.com/xxchan/rustfmt/pull/1 but it seems the recent changes in toml_edit by @epage to expose all underlying strings as RawString instead of simple &str made this PR much more difficult.

@xxchan, are you still working on this? This code was pretty much ready to go until it wasn't, so hoping to resume the work on it :)

[xxchan]

@nyurik Thanks for your work! But it seems your merge commit changed Cargo.lock too much, and thus I've manually merged main just now.

If you'd like to create a separate PR to bump toml_edit, I would happily merge it!

@xxchan, are you still working on this? This code was pretty much ready to go until it wasn't, so hoping to resume the work on it :)

I'm not working on it, and not very interested in pushing forward it (although there aren't any blockers AFAIK). But I'm happy to help if anyone wants to improve it (e.g., more tests, bugfix, etc).

[nyurik]

@xxchan i would be happy to help with that PR - do you want to maybe add me to your fork? This way i can push changes directly to your PR (again - only if you do want me to drive that PR forward)?

[xxchan]

@nyurik Instead of pushing directly to this PR, it might be better if you either create PRs to be merged into this branch, like what you already did, or create another PR to master directly to replace this PR if you want to make significant changes :)