-
Notifications
You must be signed in to change notification settings - Fork 162
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
Clean up dependencies #474
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.
LGTM but lets get the GHA ubuntu CI to pass too.
Needs to have #475 merged (and rebasing) to pass CI. |
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.
Thank you for sorting imports! huge pet peve of mine. Can this PR also add a note to DEVELOPERS.md
that requests any dependency change keep deps sorted. Seems obvious, but it's important to have something to point to so contributors aren't frustrated with unspoken rules.
I think we should add a new section on dependencies with the following language:
Dependencies
Any crate added to iroh will need to use a license compatible with ours. Any PR that introduces a new crate will require additional review time to audit the crate being introduced, including rationale on why you chose this crate, and what alternatives you considered will speed up the review process.
Crate lists in Cargo.toml
files must be kept alphabetically sorted.
c4af166
to
9dbbf33
Compare
@b5 great, added that |
These have been found by `cargo +nightly udeps --all-targets`. Leaving them in does not affect final binaries but does increase compilation time.
Makes it easier to find items and keeps things consistent, e.g. cargo-add will keep lines sorted if they already are sorted.
9dbbf33
to
286f0bf
Compare
PTAL, is rebased on main and developers docs are updated. |
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 contains two commits:
This is meant to be committed without squashing, see the commit messages for details.
If the 2nd commit is too controversial I'm happy to remove it.