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

cargo add reformats the features lists #11743

Closed
lopopolo opened this issue Feb 20, 2023 · 11 comments · Fixed by #12837
Closed

cargo add reformats the features lists #11743

lopopolo opened this issue Feb 20, 2023 · 11 comments · Fixed by #12837
Labels
A-toml Area: TOML parsing and handling C-bug Category: bug Command-add Command-remove S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing.

Comments

@lopopolo
Copy link

Problem

cargo add does not preserve formatting of TOML lists unrelated to the [dependencies] table

Steps

With the following Cargo.toml:

[package]
name = "xxx"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]

[features]
default = [
  "a",
  "b",
  "c",
]
a = []
b = []
c = []

Run:

$ cargo add directories
    Updating crates.io index
      Adding directories v4.0.1 to dependencies.

observe that the [features] table is reformatted:

diff --git i/Cargo.toml w/Cargo.toml
index ccae1b6..61c1383 100644
--- i/Cargo.toml
+++ w/Cargo.toml
@@ -6,13 +6,10 @@ edition = "2021"
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

 [dependencies]
+directories = "4.0.1"

 [features]
-default = [
-  "a",
-  "b",
-  "c",
-]
+default = ["a", "b", "c"]
 a = []
 b = []
 c = []

Possible Solution(s)

Preserve formatting of all TOML contents outside of the newly added dependency.

Notes

No response

Version

cargo 1.67.1 (8ecd4f20a 2023-01-10)
release: 1.67.1
commit-hash: 8ecd4f20a9efb626975ac18a016d480dc7183d9b
commit-date: 2023-01-10
host: x86_64-apple-darwin
libgit2: 1.5.0 (sys:0.16.0 vendored)
libcurl: 7.86.0 (sys:0.4.59+curl-7.86.0 system ssl:(SecureTransport) LibreSSL/3.3.6)
os: Mac OS 13.2.1 [64-bit]
@lopopolo lopopolo added the C-bug Category: bug label Feb 20, 2023
@lopopolo

This comment was marked as off-topic.

@lopopolo

This comment was marked as off-topic.

@weihanglo weihanglo added the A-toml Area: TOML parsing and handling label Feb 20, 2023
@weihanglo
Copy link
Member

Just did a simple bisect. This regression started from 1.66.

On an unrelated note, cargo add appends the new dependency to the end of the [dependencies] table and does not attempt to respect existing alphabetization as npm install does.

This might be a bug as well 🥲. If you have a reproducible example, please file an issue. Thank you!

@epage

This comment was marked as off-topic.

@epage
Copy link
Contributor

epage commented Feb 20, 2023

I think this was broken in #11099, specifically 7c218b1. When cargo rm removes a feature, it needs to fix up the whitespace but the existing fix instead removes user formatting

@lopopolo

This comment was marked as off-topic.

@lopopolo

This comment was marked as off-topic.

@epage

This comment was marked as off-topic.

@lopopolo

This comment was marked as off-topic.

@SummerGram
Copy link

Hi,

cargo add <a> should only append a to the dependencies only. Is it correct?

I would like to contribute this.

@weihanglo weihanglo added the S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing. label Apr 25, 2023
bors added a commit that referenced this issue May 27, 2023
fix(add): Reduce the chance we re-format the user's `[features]` table

### What does this PR try to resolve?

#11743 pointed out that we re-format the users `[features]` table when running `cargo add`  which was a bug introduced in #11099.

This reduces the chance people will run into this problem
- Reducing the scope of the `fmt` call
- Preserving formatting in a simple case

Actually removing the `fmt` case can make some common formatting cases more complex to do "right", so I'm punting on that for now.

### How should we test and review this PR?

Look at the individual commits as I show how each change improves the behavior of `cargo add`.
@epage
Copy link
Contributor

epage commented May 27, 2023

#12191 reduced the chance we'll hit this. If the user makes a change that would remove an item from the feature list (more likely with cargo remove)., then we'll still hit this.

@epage epage changed the title cargo add reformats TOML lists cargo add reformats the features lists Oct 9, 2023
epage added a commit to epage/cargo that referenced this issue Oct 17, 2023
epage added a commit to epage/cargo that referenced this issue Oct 18, 2023
bors added a commit that referenced this issue Oct 19, 2023
fix(remove): Preserve feature comments

### What does this PR try to resolve?

We've been having a hard time balancing leaving the feature list in a good looking start and preserving formatting.  With our new formatting policy (#12836), we can just choose to preserve formatting instead.

Fixes #11743

### How should we test and review this PR?

The first commit copies an existing test.  The second is where the fun begins, customizing the test for some weird cases.  The follow up commits do the slow walk for improving it.

We ended up preserving some line-trailing comments because they come after the comma and toml_edit treats that as part of the prefix of the next item.  Tracking removal of that was going to require us to determine if the newline existed in the suffix or in the next item's prefix and edit accordingly and I decided to skip that to keep this initial implementation simpler.

### Additional information
@bors bors closed this as completed in 5374e8a Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-toml Area: TOML parsing and handling C-bug Category: bug Command-add Command-remove S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants