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

Fix remaining issues with array newlines and equal sign spaces #66

Merged
merged 5 commits into from
Jan 10, 2024

Conversation

DevinR528
Copy link
Owner

Well, this fixes all the issues, I think. If you think this is reasonable, I would just add docs in the readme about the two new tomlfmt options (max_array_line_len and indent_count) and it's ready to go. I could add the docs to the readme and it probably won't take me a whole year haha.

@jplatte
Copy link
Collaborator

jplatte commented Dec 8, 2023

Hm, I think Ruma never explicitly opted into formatting, just sorting. Is it on by default? Running cargo sort --workspace --grouped --order package,lib,features,dependencies,target,dev-dependencies,build-dependencies in Ruma with the crates.io release doesn't reformat anything, running it with the state at main makes every array single-line and running it with the state at this branch makes every array multi-line (max line length seems broken and it fails to add / removes trailing commas for the last array member).

@DevinR528
Copy link
Owner Author

I had a copy/paste error, and with that fixed now, it breaks arrays that are longer than 80 bytes (yes, this is just the array part of the string). I'm pretty sure with the older version the formatting was less aggressive, we just kept more of the user's formatting. With the changes now it only reformats 2 missing commas and a multiline array into a single because it is under the 80 bytes. We could make the default 60 and the changes to Ruma's Cargo.toml would just be the 2 commas.

@jplatte jplatte changed the title Fix reamaining issues with array newlines and equal sign spaces Fix remaining issues with array newlines and equal sign spaces Dec 8, 2023
Copy link
Collaborator

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

I just tested this and it looks pretty good! Is it supposed to always say that it has rewritten the file, even if no changes were done, when running in non-check mode?

Also, formatting currently removes trailing commas on multiline arrays, which I find wrong. Do you agree?

Finally, in workspaces where one crate has the same name as the workspace directory itself, it's a bit confusing to see two Checking main-crate lines. How about either changing the messages to refer to the proper file path of the file that was checked, or printing a different message when the Cargo.toml doesn't contain a [package] section?

In any case, approving since this PR is a clear improvement so I wouldn't mind addressing all of those things separately from it.

@DevinR528
Copy link
Owner Author

Is it supposed to always say that it has rewritten the file, even if no changes were done, when running in non-check mode?

I'd say this is a bug, it probably always has... I think.

Also, formatting currently removes trailing commas on multiline arrays, which I find wrong. Do you agree?

Agreed, a few of the multiline formatting choices toml_edit made are strange.

I like the idea about either printing file paths or check for [package] section and print different message.

At some point we can file issues for all these!

@DevinR528 DevinR528 merged commit 55ec890 into main Jan 10, 2024
4 checks passed
@DevinR528 DevinR528 deleted the fix-array-newline branch January 10, 2024 23:48
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

Successfully merging this pull request may close these issues.

2 participants