-
-
Notifications
You must be signed in to change notification settings - Fork 144
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
Run cargo sort-ck on the workspace #544
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, except for ruma/Cargo.toml
. Does cargo-sort-ck also do the reordering? In that case shouldn't it be called cargo sort
(with --check
being an optional flag)?
ruma/Cargo.toml
Outdated
ruma-common = { version = "0.5.0", path = "../ruma-common" } | ||
ruma-identifiers = { version = "0.19.0", path = "../ruma-identifiers", features = ["serde"] } | ||
ruma-serde = { version = "0.3.1", path = "../ruma-serde" } | ||
|
||
ruma-client = { version = "=0.5.0-alpha.2", path = "../ruma-client", optional = true } | ||
ruma-events = { version = "=0.22.0-alpha.3", path = "../ruma-events", optional = true } | ||
ruma-signatures = { version = "0.7.0", path = "../ruma-signatures", optional = true } | ||
|
||
ruma-api = { version = "=0.17.0-alpha.4", path = "../ruma-api", optional = true } | ||
ruma-appservice-api = { version = "=0.2.0-alpha.3", path = "../ruma-appservice-api", optional = true } | ||
|
||
ruma-client = { version = "=0.5.0-alpha.2", path = "../ruma-client", optional = true } | ||
ruma-client-api = { version = "=0.10.0-alpha.3", path = "../ruma-client-api", optional = true } | ||
|
||
ruma-common = { version = "0.5.0", path = "../ruma-common" } | ||
ruma-events = { version = "=0.22.0-alpha.3", path = "../ruma-events", optional = true } | ||
ruma-federation-api = { version = "=0.1.0-alpha.2", path = "../ruma-federation-api", optional = true } | ||
ruma-identifiers = { version = "0.19.0", path = "../ruma-identifiers", features = ["serde"] } | ||
ruma-identity-service-api = { version = "=0.1.0-alpha.1", path = "../ruma-identity-service-api", optional = true } | ||
ruma-push-gateway-api = { version = "=0.1.0-alpha.1", path = "../ruma-push-gateway-api", optional = true } | ||
ruma-serde = { version = "0.3.1", path = "../ruma-serde" } | ||
ruma-signatures = { version = "0.7.0", path = "../ruma-signatures", optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense. The entries were grouped, now entries from different groups have been switched around. I want the dependencies sorted inside the group, but definitely not across groups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I would imagine this is common enough to warrant a new --grouped
flag or something like this I'll add it to cargo-sort-ck
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to keep "full" sorting as the default, I would recommend that it should at least remove the grouping entirely then (remove the empty lines between dependencies), the current output is just confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the --grouped
option and a config file this now works perfectly. I'm not crazy about the config file but that many cli flags is worse 🤷
It's the opposite, the default behaviour is just to exit with a nonzero and print an error. When |
That's the opposite default from |
Yeah, open an issue. The reason was it satisfied dtolnay/request-for-implementation#29 which was why I wrote it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the review comment below, please remove the extra config file as discussed.
I'm just waiting on toml-rs/toml#104 to be merged then I can publish a new version of cargo sort-ck and add it to CI, but first I wanted to see what everyone thinks of the output?