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

Added logic to preserve carriage returns when updating the manifest file #14983

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ranger-ross
Copy link
Contributor

What does this PR try to resolve?

This is an attempt to preserve /r/n line endings in Cargo.toml during cargo add/remove by checking if it already had at least one /r/n.

See #14863

Additional information

For testing, it appears that snapbox will normalize line ending so the tests will directly compare the file contents.

@rustbot
Copy link
Collaborator

rustbot commented Dec 26, 2024

r? @epage

rustbot has assigned @epage.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-manifest Area: Cargo.toml issues S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 26, 2024
@ranger-ross

This comment was marked as outdated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this does what we want. However, I am not sure if this should be in toml_edit or in Cargo. Found one relevant comment from an issue: toml-rs/toml#752 (comment).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#14863 isn't marked as S-accepted. I would generally recommend discussing there, and leave this for implementation only dicussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't marked as S-accepted. I would generally recommend discussing there, and leave this for implementation only dicussion.

ah my bad 😅 I was looking through C-bug issues for small bugs to work on to learn/get some experience with the Cargo codebase. I'll be sure to stick to S-accepted or at least discuss further on the issue before starting/opening a PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be auto-converted, if core.autocrlf = true is set in git configuration on Linux. So the test always passes even without this patch. I haven't tested it locally though. Maybe we should have a .gitattributes file and set the file eol=crlf?

Slightly related, in contributing doc we recommend splitting new tests into its own commit, and it still passes CI. Then followed by a fix and a test update. The test change in the second commit tell that that the bug is actually there, and the patch really fixes it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-manifest Area: Cargo.toml issues S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants