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

Review Cargo.toml manifest formatting #188

Open
joshtriplett opened this issue Oct 17, 2023 · 10 comments
Open

Review Cargo.toml manifest formatting #188

joshtriplett opened this issue Oct 17, 2023 · 10 comments

Comments

@joshtriplett
Copy link
Member

The cargo team is interested in having cargo fmt format the manifest. We should review the manifest formatting and make sure it's what we want it to be for automatic formatting.

@calebcartwright
Copy link
Member

@gilescope
Copy link

@joshtriplett is there any major blocker why we can't allow what's there to be opt in on nightly? Big businesses have hundreds (sometimes thousands) of crates and frankly any vaguely sensible auto-formatting of Cargo.tomls is better than no formatting. I would really really like to see this functionality being available in nightly (it's under a rustfmt configuration flag already). When people opt into auto-formatting, they expect the formatting to get better over time as defaults change (and can configure things if they have different opinions). This isn't a huge barrier to rust's enterprise adoption, but it is friction that we can and should remove. By allowing it in nightly we will get a lot more feedback (hopefully in the form of PRs) quickly than any other route.

(This has been on the top of my xmas list two years running now. I really hope Santa is coming to town.)

@calebcartwright
Copy link
Member

The Style Team does not govern the Rustfmt team, and nor does it determine the if/when/what of Rustfmt features. As such I'd ask that we keep this issue on-topic for discussion of the actual Style rules.

@calebcartwright
Copy link
Member

For awareness, Zulip thread with some discussion amongst the Cargo team around the various rules - https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/.60Cargo.2Etoml.60.20style.20guide

it would still be nice to at least get a summarized form of their latest thinking, though a PR with specific language to https://github.com/rust-lang/rust/blob/master/src/doc/style-guide/src/cargo.md would be ideal 🙏

Josh given the many hats you wear and role on both teams, I'm guessing you may have more insight on the specifics?

@calebcartwright
Copy link
Member

i'm nominating in the hopes that there are/will be enough specifics to discuss at the next style meeting. feel free to remove the nomination if that's not the case though

@joshtriplett
Copy link
Member Author

The primary feedback that I saw on the Cargo.toml was that putting description last was surprising to the Cargo team.

Otherwise, I think what we have is probably fine.

@joshtriplett
Copy link
Member Author

rust-lang/rust#120072

@epage
Copy link

epage commented Jan 17, 2024

Otherwise, I think what we have is probably fine.

Some items I noticed that weren't captured:

  • We should call out the things discouraged by the TOML spec, like out-of-order tables
  • ehuss called out that tables sometimes have blank lines inside of them. This is especially common for [features]
  • This encourages sorting of fields within a table but the only table I can think of where that might make sense is dependencies.
    • [featutes] tends to be logically grouped
    • A lot of tables have at least some keys with a logical ordering (like name being first) or some keys are closely related and should be next to each other
      • If we want to capture these specific in the style, rather than be agnostic on it, the thread had a lot of good suggestions
  • ehuss pointed out that "The use of / in a license is deprecated. I would either drop the mention of /, or specifically say "use OR, not /"."
  • No comment is made about when to use dotted keys in fields (rust-version.workspace = true)
  • [workspace] isn't taken into account (top, bottom, somewhere else?)

EDIT: Aug 2024 updates from https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/.60Cargo.2Etoml.60.20style.20guide

  • [lints] might want to be sorted by (priority, level, lint_name)
  • Should the default be to use "" or ''?

Example of why escaping came up:

[lints.rust]
unexpected_cfgs = { level = "warn", check-cfg = ["cfg(has_foo)", "cfg(has_bar, values(\"yes\", \"no\"))"] }   # escaping
unexpected_cfgs = { level = "warn", check-cfg = ["cfg(has_foo)", 'cfg(has_bar, values("yes", "no"))'] }  # no escaping but inconsistent
unexpected_cfgs = { level = 'warn', check-cfg = ['cfg(has_foo)', 'cfg(has_bar, values("yes", "no"))'] }  # consistent

@joshtriplett
Copy link
Member Author

From discussion in today's @rust-lang/cargo meeting: We also need to add text about the process for adding new Cargo features that might need style (e.g. nominating an issue with I-style-nominated and getting style guide text added before stabilization).

Also, in addition to name and version being at the top of package, workspace should be as well.

@gilescope
Copy link

gilescope commented Feb 27, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants