-
Notifications
You must be signed in to change notification settings - Fork 736
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
Reorder dependencies' keys #6967
base: master
Are you sure you want to change the base?
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.
totally agree on that
I hope we can merge this ASAP. Then I can happily work on other PRs. cc @bkchr @ggwpez @kianenigma 🙏 |
Yes, very much like this. For FRAME pallets, we also alleviate this by encouraging users to use |
@@ -17,10 +17,10 @@ codec = { workspace = true, default-features = true } | |||
url = { workspace = true } | |||
|
|||
# Substrate |
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.
Not for this PR maybe, but I'm also not a huge fan of dependencies being separated with comments like this. If we're sorting them now I think it's a good time to remove them
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.
I also thought about this before. I agree you. Since they have already been "prefixed". It's easy to distinguish.
bot fmt |
@bkchr https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7944682 was started for your command Comment |
@bkchr Command |
It doesn't make sense to only reorder the features array.
For example:
This makes it hard for me to compare the dependencies and features, especially some crates have a really really long dependencies list.
This makes my life easier.