DevinR528/cargo-sort

Applying sort-ck to the cargo workspace root results in corrupted Cargo.toml

Closed this issue ยท 9 comments

eupn commented

First of all, thank you for this tool! It is really handy.

Unfortunately, it doesn't seems to work with cargo workspaces properly.

Given the workspace manifest:

[workspace]
members = [
  "first",
  "second",
  "third",
  "fourth",
  "fifth",
  "sixth",
]

Expected behaviour:

  1. Sort members of the workspace Cargo.toml
  2. Sort keys in Cargo.toml of workspace members

Actual behaviour:

  • Workspace's manifest is corrupted:
[workspace]
members = [
"first",=
"second",=
"third",=
"fourth",=
"fifth",=
"sixth",=
]=
  • Workspace crate's Cargo.tomls weren't analysed

Ehh yea that's no good at all. I've started working on fixing this I can either do it quickly or do it right (this would involve returning an enum, every parse method could return almost any toml type). Of the list that's sorted now only [workspace] is special like this so I may just go with the quick fix unless you can think of any other headers that can have non key value pairs under them.
Currently this is what's checked

"dependencies",
"dev-dependencies",
"build-dependencies",
"workspace",

If so I can make this a special case

[workspace]
members = [
  "first",
]
exclude = [  "other_stuff" ]

I also fixed the doc link and will push everything at once thanks for finding both these bugs!

eupn commented

There's no rush here, it's best to do it right, in the long run. I can try to fix it and file a PR if you give me some input on your vision on how do you wish this to be fixed/improved!

Yea that be awesome! My idea to handle this correct would be to have TomlItems hold a vec of enums instead of just TomlKVPair to represent both a struct that would basically be another table (some type you create) and TomlItem. The new struct would have a similar Parse trait and in the parse_items method there would be some condition (header == "member") that calls parse on the new struct. This could be moved to the enum also. For the sorting if the new type is found in sort_items the new struct could be called to sort maybe?

enum TomlItem {
    KvPair(...),
    List(TomlInlineTable),
}

struct TomlInlineTable {
    heading: String,
    items: Vec<TomlKVPair?>

This is my rough idea, if you can think of a better way I'm all for it. these names are pretty bad but thats always a hard part.

Update:
I have started writing a full tokenizer using rowan all I have to do now is make conversion methods to turn the tree into usable structs here is the WIP. I really enjoy writing parsing stuff so this may be serious over kill but I was also thinking of making a cargo fmt like tool for toml toml fmt?

@eupn sorry that took me so long I finally got around to moving the parsing and formatting into this project. Try it out it should have solved this issue and a few other tricky spots when it came to sorting!

eupn commented

@DevinR528 it works, thanks! Although, formatting isn't preserved in the result:

Original manifest:

[workspace]

members = [
    "3",
    "4",
    "2",
    "1",
]

Is turned into:

[workspace]
members = [ "1",
"2",
    "3",
    "4",
]

I'm pretty sure I know why it did that, I should get a fix in today! thanks for testing it.

The header will always only have one \n after the ] but the indent and no new line after the [ are bugs.

@eupn got it

[workspace]
members = [
    "1",
    "2",
    "3",
    "4",
]
eupn commented

@DevinR528 can confirm that it's working now. Thanks!