feat: support formatting Cargo.toml

[xxchan]

From the disscussion in #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 #4091 (comment)

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 #4091 (comment)

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

[ytmimi]

@xxchan Thanks again for submitting this PR. It's a really cool feature that I hope we continue to work on.

Overall the implementation seems fine to me, and nothing is jumping out as obviously problematic. I think any issues with the implementation will reveal themselves with more thorough testing, so take a look at some of the notes about getting format_cargo_toml onto a code path that will be called by rustfmt, and also take a look at the note about updating get_test_files to find .toml files in tests/source and tests/target.

Also just a few minor call outs to replace println! with log::debug!, and to remove commented out code when possible.

Great work on this so far!

[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 #4091 (comment), 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 <#5240 (comment)>, 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? 😄

[gilescope]

🎅 It's top of my Christmas list that with some flag set, nightly can format Cargo.toml files. I've been a really good boy all year.

[calebcartwright]

While this has admittedly not been a priority for us on the Rustfmt team anyway, I also want to make sure that those interested in this feature are aware that the Cargo team have some concerns about the existing Style Guide prescriptions for the toml file and that there's still discussion to be had between the Cargo and Style teams about what the formatting rules should be.

[clarfonthey]

If that's the case, perhaps it would be okay to put this under an unstable option (for example, cargo_toml = true) so that the existing code can be merged without stabilising the formatting? Assuming that the consensus is that it's okay for this code to live in rustfmt, just, not certain what it should do.

[nyurik]

I second that if we are simply uncertain of the formatting details, but certain that we need it (as seems to be the consensus), I do think it should be merged under the nightly flag, same as --config imports_granularity=Module. Options for the name:

• include_toml
• format_toml
• toml
• ...?

[xxchan]

If that's the case, perhaps it would be okay to put this under an unstable option

a config format_cargo_toml is already added https://github.com/rust-lang/rustfmt/pull/5240/files#diff-7bd4f14a75a37432af3ab896bbbe38f88428b490b5d76ed1586f7f473a745e98

the Cargo team have some concerns about the existing Style Guide prescriptions for the toml file and that there's still discussion to be had between the Cargo and Style teams about what the formatting rules should be.

Understand. I also feel some rules are not good enough, and some are missing. But I think

• This implementation is extensible and very easy to change
• With a working implementation for the current style guide, maybe it's easier to find what can be improved.

[xxchan]

the Cargo team have some concerns about the existing Style Guide prescriptions for the toml file and that there's still discussion to be had between the Cargo and Style teams about what the formatting rules should be.

ref to rust-lang/style-team#188

[gilescope]

Just like formatting of rust code has changed over time, it's expected that formatting of Cargo.toml files would change over time (and double expected when the feature is only available in nightly). I have no doubt that releasing it to nightly will quickly lead to feedback around what the default formatting style should be. This is what nightly is for is it not?

[calebcartwright]

To be clear, I'm trying to provide transparency about additional challenging external factors above and beyond the fact that this is an extremely low priority for a small, stretched team. I'm not trying to persuade anyone of anything, nor get dragged into a debate.

Just like formatting of rust code has changed over time

Fwiw, it hasn't. The Style Guide has been set in stone from the outset, as have the associated formatting rules. It wasn't until RFC 3338 (from 2022) that a process to change formatting even came into existence.

---

It's just not a particularly efficient use of our time to review code implementing formatting rules and that the associated test content based on those rules, for rules that are going to change.

(edit: typo fix)

[xxchan]

To be clear, I'm trying to provide transparency about additional challenging external factors above and beyond the fact that this is an extremely low priority for a small, stretched team. I'm not trying to persuade anyone of anything, nor get dragged into a debate.

Totally understand, and glad that you can express this explicitly.

It's just not a particularly efficient use of our time to review code implementing formatting rules and that the associated test content based on those rules, for rules that are going to change.

FWIW, @ytmimi has spent time reviewing this several rounds, and thought we're on the right track. So I just felt a little confused why to hold off. This seems also not very efficient use of time.

Anyway, I'm not urgent at all to get this landed. But just want to say sth since we got updates from the maintainers' side. 😄

[calebcartwright]

FWIW, @ytmimi has spent time reviewing this several rounds, and thought we're on the right track. So I just felt a little confused why to hold off. This seems also not very efficient use of time.

Understood. The Cargo team's objections are recent, and the prior rounds of review precede those objections.

This has admittedly been a lower priority from the outset, but it was something we were still trying to slowly advance. However, these new objections are now a blocker, and I'm simply communicating that blocker.

[epage]

If this is about the parts of the style that are undecided, how about we focus on the core infrastructure and easily agreed to items so this PR can be small and we can more easily iterate over time. I also assume that the area where the cargo team pushed back are likely to be things people would want configurable so they can be added incrementally over time behind settings.

[epage]

These are two areas where more style discussions need to happen

[calebcartwright]

I'm really trying to avoid getting into broader discussion as well as the specifics of what/why/etc. because I think discussion on PRs should be largely centered review of the "how", the specifics of proposed implementation changes.

Ultimately, this is not something I'm comfortable moving ahead with at this time, neither in whole nor in part, while we have other conversations circling elsewhere including moving some of the subcommands (like cargo-fmt) into Cargo and large unanswered questions around things like sorting.

I know people want this feature, I get it and I get why. For those that just want some/any type of formatting of their Cargo.toml files, several readily available options were previously shared on #4091.

However, this is something net-new and trying to force through for the sake of having something "now" from rustfmt is only going to introduce churn, both on users and on rustfmt itself, and we're not going to do it. When we do, it will be something we generally feel will be "right" and at least aligned with the anticipated path of the Style Rules.

If that makes me the Grinch instead of Santa, then so be it 😅

[epage]

I'm really trying to avoid getting into broader discussion as well as the specifics of what/why/etc. because I think discussion on PRs should be largely centered review of the "how", the specifics of proposed implementation changes.

To be clear, I was trying to highlight what could be dropped to make this easier to move forward, from a cargo team member perspective, not to get into the broader discussion.

Ultimately, this is not something I'm comfortable moving ahead with at this time, neither in whole nor in part,

Please reconsider this. Focusing on a more incremental approach should make your life much easier and gets people benefits now, rather than waiting for some future date. We can pick less churn-heavy areas to move forward.

including moving some of the subcommands (like cargo-fmt) into Cargo and large unanswered questions around things like sorting.

I thought you said "no" to moving it?

As for sorting, if we leave that out, then it isn't a problem, right?

[calebcartwright]

I'm going to redirect conversation about design/strategy/timing/etc. back to the issue and will respond there

[clarfonthey]

However, this is something net-new and trying to force through for the sake of having something "now" from rustfmt is only going to introduce churn, both on users and on rustfmt itself, and we're not going to do it. When we do, it will be something we generally feel will be "right" and at least aligned with the anticipated path of the Style Rules.

If that makes me the Grinch instead of Santa, then so be it 😅

My particular concern here, speaking of someone who understands what it's like to contribute changes to large projects on my own time, is that it's incredibly frustrating to leave PRs in limbo like this. I'm fine with stripping out changes that could be changed/cause churn, I'm fine with deciding that you just don't want this change before design is done. What I'm not okay with is a change being open for almost two years in limbo.

It takes a lot of effort to rebase and review a change like this as it gets scrutinised, and it's especially frustrating to continue to have a change left open for months because it's in this weird limbo of not prioritised but not undesired.

The reason why I proposed merging under an unstable option is because it would mean this PR is closed and then additional work can happen in another PR.

Like, I understand the situation this puts you in as a maintainer, but I'm speaking from experience; either:

1. Merge it as-is
2. Provide criteria for what form it would need to be, to be merged
3. Close it

None of this "we don't know what the criteria is, don't want to work on it right now, but like some of what this is right now." That just makes everyone feel worse about it, both maintainers and contributors. IMHO, either decide if there's some portion of this you'd be comfortable merging now (and request those specific changes, likely involving removing pieces) or close this and tell people where they can follow the discussion in the meantime.

[glandium]

Does this produce Cargo.tomls like those cargo publish itself produces?

[epage]

@glandium while there is work to define and enforce the style for Cargo.toml, I do not expect the cargo team will adjust cargo publish to make the Cargo.toml apply. The file at that point is mostly for machine consumption. This would be similar to cargo fix generating code that needs a cargo fmt run applied to it.

[TerakomariGandesblood]

Any progress after all this time? What is stopping this work?

[LGLO]

This would be very nice addition to the ecosystem!

[calebcartwright]

This PR is blocked, as has been noted in multiple comments and with the label on this PR.

We, the rustfmt team are leaving the PR open because it's as good an implementation as any to pick up if and when the upstream blockers are removed. If the author wants to close the PR that's certainly their prerogative and an option they can take at any point.

I've locked the conversation on this PR because it's been attracting a number off-topic comments that do not offer any concrete feedback on the implementation nor remove the upstream blockers.

Those comments are adding noise and length to the PR which exacerbate the common open source issue of people commenting without reading prior comments, and it's also unnecessarily triggering notifications for those subscribed in order to receive substantive updates.