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

Unify the styling of dependencies in Cargo.toml #47

Merged
merged 12 commits into from
Oct 3, 2021

Conversation

liuchengxu
Copy link
Contributor

Basically, my personal taste for the dependencies styling, the main idea is : non-substrate deps, sc-*, sp-*, frame-*/pallet-*, subspace deps. Feel free to suggest another one, but a unified style is always good for collaboration.

Putting the dep items in batches also brings more convenience for the batch editing operation in a text editor.

No code changes, only styling stuff. I believe it works if it compiles.

Related #44.

Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

General comments:

  • No need to split dependencies into multiple sections and prefix with # Subspace, one list sorted alphabetically is fine
  • [dependencies.X] is actually preferred for non-trivial dependencies in crates that are not inherently related to Substrate
  • Please don't change crate authors (code is licensed by the company, but authored by individuals)

crates/sp-consensus-spartan/Cargo.toml Show resolved Hide resolved
@liuchengxu
Copy link
Contributor Author

The benefit for multiple sections is that we can separate out the client-only and runtime-related crates clearly, runtime means it should compile under no_std and we could spot the missing no_std feature easier aside from relying on the compiler :D.

I agree that [dependencies.X] is more suitable for complex cases, but I think we should stick with one style for the common simple cases.

@nazar-pc
Copy link
Member

nazar-pc commented Oct 2, 2021

we can separate out the client-only and runtime-related crates clearly

I don't see much benefit in that, it is pretty clear that if you have something under runtime or deeper in dependencies, it should be no_std-compatible or else it doesn't compile, in all other cases it no_std has no relevance, so it is not much of a problem. I would prefer to stick to a simpler rule with a single section and alphabetically sorted dependencies.

What do you think about using https://github.com/dprint/dprint in CI and automate the process of maintaining the formatting of Cargo.toml files?

@liuchengxu
Copy link
Contributor Author

liuchengxu commented Oct 2, 2021

Well, for me, the multiple sections(e.g., Subspace section) allow me to know which crates are not using the upstream directly but written on our own at the first glance, but I'm ok if you insist that only one section is enough.

I don't have a strong opinion on dprint as toml seemingly does not have an official formatter like rustfmt. It's good to me as long as it helps us achieve a sort of consistent style that is acceptable to all of us.

@nazar-pc
Copy link
Member

nazar-pc commented Oct 3, 2021

Yes, let's use a single section for now and this should be good to go.

dprint was suggested in rust-lang/rustfmt#4091, not critical for now

Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Node dependencies still have multiple sections, looks good otherwise

@nazar-pc nazar-pc merged commit 3d10233 into autonomys:main Oct 3, 2021
@nazar-pc
Copy link
Member

nazar-pc commented Oct 3, 2021

Thank you!

@liuchengxu liuchengxu deleted the clean-up-deps branch October 6, 2021 02:07
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