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

Clean up dependencies. #28382

Merged
merged 1 commit into from
Oct 10, 2024
Merged

Clean up dependencies. #28382

merged 1 commit into from
Oct 10, 2024

Conversation

mikebenfield
Copy link
Collaborator

Remove unused deps. Use workspace deps where appropriate.

@mikebenfield mikebenfield requested a review from d0cd October 5, 2024 03:53
Cargo.toml Show resolved Hide resolved
Copy link
Collaborator

@d0cd d0cd left a comment

Choose a reason for hiding this comment

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

Do you forsee any issues with maintainability by moving all of these workspace dependencies? Curious, does it reduce build time?

@mikebenfield
Copy link
Collaborator Author

I doubt it reduces build times generally. Maybe in some weird cases where some crates were getting redundantly built multiple times? Not sure.

IMO the main advantage is just keeping the versions and feature flags in one place. We bump the version of Leo or a dependency, we don't have to search across a ton of different Cargo.toml files to make changes.

Cargo.toml Outdated Show resolved Hide resolved

[dependencies.indexmap]
version = "1.9"
features = [ "serde-1" ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the workspace dependency for indexmap need the serde-1 feature?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My understanding from reading indexmap docs is that serde-1 is sort of deprecated now, and instead we're supposed to use serde.

These release notes mention serde-1 being removed (although for a later version than we're using)

https://github.com/indexmap-rs/indexmap/blob/master/RELEASES.md

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's upgrade to 2.0.0 and use serde.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I switched the remove calls to remove_shift, as discussed, although that does potentially change behavior.

@mikebenfield mikebenfield force-pushed the dependencies branch 3 times, most recently from c7319af to f935e01 Compare October 10, 2024 04:30
Remove unused deps. Use workspace deps where appropriate.
Copy link
Collaborator

@d0cd d0cd left a comment

Choose a reason for hiding this comment

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

LGTM!

@mikebenfield mikebenfield merged commit a55dfe2 into mainnet Oct 10, 2024
13 checks passed
@mikebenfield mikebenfield deleted the dependencies branch October 10, 2024 19:56
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