test: additional tests for format_cargo_toml

[CBenoit]

Here are additional test cases for the "format_cargo_toml" feature

[CBenoit]

Should be sorted alphabetically

Suggested change

	miniz_oxide = { version = "0.7.0", optional = true, default-features = false }
	addr2line = { version = "0.21.0", optional = true, default-features = false }
	addr2line = { version = "0.21.0", optional = true, default-features = false }
	miniz_oxide = { version = "0.7.0", optional = true, default-features = false }

[CBenoit]

(Controversial) result for this section is not very satisfying:

1. newlines were useful to group related features together
2. the comment about the "std_detect" group is now below some the "std_detect_*" features, though the expectation was for it to be above "std_detect_dlsym_getauxval"

I’m not sure we can do much for 2); after all the comment was indeed above "std_detect_file_io", but for 1), maybe separated "groups" should stay separated?

That is, maybe something like should be considered "properly formatted":

[features]
backtrace = [
    "gimli-symbolize",
    "addr2line/rustc-dep-of-std",
    "object/rustc-dep-of-std",
    "miniz_oxide/rustc-dep-of-std"
]
gimli-symbolize = []

# Make panics and failed asserts immediately abort without formatting any message
panic_immediate_abort = ["core/panic_immediate_abort"]

# Enable std_detect default features for stdarch/crates/std_detect:
# https://github.com/rust-lang/stdarch/blob/master/crates/std_detect/Cargo.toml

std_detect_dlsym_getauxval = ["std_detect/std_detect_dlsym_getauxval"]
std_detect_env_override = ["std_detect/std_detect_env_override"]
std_detect_file_io = ["std_detect/std_detect_file_io"]

Maybe this could be configured with a new key in the rustfmt.toml file.

[xxchan]

result for this section is not very satisfying

Indeed, but I'm also unsure how to improve it. For 1) How can we define "grouping" when the items need to be sorted? For 2) I think it can be manually tweaked after the sorting 🤪.

[CBenoit]

How can we define "grouping" when the items need to be sorted?

I would define it as "items separated by newlines":

# First group
backtrace = [
    "gimli-symbolize",
    "addr2line/rustc-dep-of-std",
    "object/rustc-dep-of-std",
    "miniz_oxide/rustc-dep-of-std"
]
gimli-symbolize = []

# Second group
panic_immediate_abort = ["core/panic_immediate_abort"]

# Third group
# "Comment-only" group with no key

# Fourth group
std_detect_dlsym_getauxval = ["std_detect/std_detect_dlsym_getauxval"]
std_detect_env_override = ["std_detect/std_detect_env_override"]
std_detect_file_io = ["std_detect/std_detect_file_io"]

Keys within a group are sorted alphabetically.

There are several avenues:

• Make it the default behavior
• Make it configurable
• Don’t support this

That being said, here is probably not the best place to discuss this. This should probably be tracked with a follow-up issue when your PR lands on nightly.

For 2) I think it can be manually tweaked after the sorting 🤪.

Absolutely 😆

[CBenoit]

You were probably aware of this, but there is an extra white space here:

Suggested change

	[target.'cfg(any(all(target_family = "wasm", target_os = "unknown"), target_os = "xous", all(target_vendor = "fortanix", target_env = "sgx")))' .dependencies]
	[target.'cfg(any(all(target_family = "wasm", target_os = "unknown"), target_os = "xous", all(target_vendor = "fortanix", target_env = "sgx")))'.dependencies]

[xxchan]

Thanks! Would you like to take a look at the little bugs found? If not, I will take a look later.

[xxchan]

nit: The test case is not minimal. It might be better to comment what's the intention of the test (and/or minimize it?)

[CBenoit]

You’re right, it’s more like an integration test looking like a Cargo.toml from an actual project and the name is not reflecting that.
I’ll minimize this into multiple tests.

[xxchan]

result for this section is not very satisfying

Indeed, but I'm also unsure how to improve it. For 1) How can we define "grouping" when the items need to be sorted? For 2) I think it can be manually tweaked after the sorting 🤪.

[CBenoit]

Thanks! Would you like to take a look at the little bugs found? If not, I will take a look later.

I’m willing to look into this more in depth, but I suspect one of the bug, the extra white space, may be addressed by #2, and I would rather wait for this to be merged before I try to modify the implementation and end up fixing conflicts.

Also, if these are not blockers for rust-lang#5240, I would be more comfortable with tracking and addressing them separately (I’ll open issues for that). I feel it’s easier for the rustfmt team to review smaller chunks instead of making rust-lang#5240 even bigger that it is already.

What do you think?

[xxchan]

#2 is merged.

I prefer fixing minor issues earlier if we find them. The changes should be minimal and won't bring in much new code.

[CBenoit]

#2 is merged.

I prefer fixing minor issues earlier if we find them. The changes should be minimal and won't bring in much new code.

Okay, I’ll check again now that #2 is merged