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

Reorder wallet imports #2953

Merged
merged 3 commits into from
Nov 28, 2024
Merged

Conversation

Neotamandua
Copy link
Member

@Neotamandua Neotamandua commented Nov 12, 2024

#2947 for rusk-wallet, test-wallet & wallet-core

I'm intentionally leaving all pub use, mod and other exports as they are, because it's still up for discussion how we want to order them.

@Neotamandua Neotamandua force-pushed the neotamandua/reorder_wallet_imports branch from 60239c4 to 551fe62 Compare November 12, 2024 16:48
@Neotamandua Neotamandua marked this pull request as draft November 12, 2024 17:29
@Neotamandua Neotamandua force-pushed the neotamandua/reorder_wallet_imports branch from 551fe62 to c407992 Compare November 12, 2024 17:47
@Neotamandua Neotamandua force-pushed the neotamandua/reorder_wallet_imports branch from c407992 to 4ba25ae Compare November 25, 2024 15:11
@Neotamandua Neotamandua force-pushed the neotamandua/reorder_wallet_imports branch 2 times, most recently from f78131b to 7653926 Compare November 25, 2024 15:41
@Neotamandua Neotamandua marked this pull request as ready for review November 25, 2024 15:42
@Neotamandua Neotamandua force-pushed the neotamandua/reorder_wallet_imports branch from 7653926 to 4591689 Compare November 25, 2024 16:14
Copy link
Member

@moCello moCello left a comment

Choose a reason for hiding this comment

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

LGTM

Two general comments but none blocking:

  1. Do we want to separate the workspace deps from the external deps or mix them in one block?
  2. I think the rusk-wallet deps in the rusk-wallet binary should be separated from the external deps

@Neotamandua Neotamandua merged commit da006b5 into master Nov 28, 2024
15 checks passed
@Neotamandua Neotamandua deleted the neotamandua/reorder_wallet_imports branch November 28, 2024 10:31
@Neotamandua
Copy link
Member Author

LGTM

Two general comments but none blocking:

1. Do we want to separate the workspace deps from the external deps or mix them in one block?

2. I think the `rusk-wallet` deps in the `rusk-wallet` binary should be separated from the external deps

I would prefer not to separate them further. Even if they are logically tied, they are still external dependencies at some point. (Hovering over rusk_wallet in main.rs for example tells you extern crate rusk_wallet)

For imports, I would only distinguish between std, external, and crate. And to me, anything that is external is something that cannot be referenced by self, super, or crate.

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