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

Replace external dependencies with re-exports #1154

Open
wants to merge 4 commits into
base: next
Choose a base branch
from

Conversation

varun-doshi
Copy link
Contributor

@varun-doshi varun-doshi commented Feb 17, 2025

@PhilippGackstatter
Copy link
Contributor

PhilippGackstatter commented Feb 24, 2025

@varun-doshi What is the state of this PR 🙂? Would you like to continue working on this?
I think it might make sense to do one PR for one or maybe two crates at a time to avoid too many conflicts.

@varun-doshi
Copy link
Contributor Author

@varun-doshi What is the state of this PR 🙂? Would you like to continue working on this? I think it might make sense to do one PR for one or maybe two crates at a time to avoid too many conflicts.

Apologies for the delay @PhilippGackstatter . I will get this done in the next few hours.
Also yes, good idea to split this up into multiple PRs.
I think this PR is ready to review then(will resolve the merge conflicts)

Will raise another Pr for the other 3 crates

@varun-doshi
Copy link
Contributor Author

On second thought, if you'd like I can split this PR into 2 separate ones as well considering this PR has 40 file changes

to clarify:
miden-object: 28 file changes
miden-tx:12 file changes

Let me know how you'd like me to proceed @PhilippGackstatter

Comment on lines +1 to 4
use super::{AccountId, Asset, Word};
use crate::note::{
scripts, NoteError, NoteExecutionMode, NoteInputs, NoteRecipient, NoteTag, NoteType,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not import these from super. If we now remove the Word import from crates/miden-lib/src/note/mod.rs then this file would also be affected, and those should ideally be independent.
I would prefer using crate:: imports.

I think I would generally avoid super imports, as they are more likely to break during a refactor.

Could you rewrite super imports to crate or the corresponding crate, if it's not exported from the crate? For example here, AccountId should be imported from miden_objects because that's where it is originally defined and we don't re-export it from the miden-lib root, which is good.

If you're using VS Code you can set "rust-analyzer.imports.prefix": "crate" so it will prefer importing from crate::.

@PhilippGackstatter
Copy link
Contributor

Let me know how you'd like me to proceed @PhilippGackstatter

Since it's all in this PR now, it's fine to keep it as one.

I think this PR does more than described in #1145. The goal is to only rewrite imports that are re-exported from the crate itself.

For example in miden-objects, we re-export assembly::Assembler:

pub mod assembly {
pub use assembly::{
mast, Assembler, AssemblyError, DefaultSourceManager, KernelLibrary, Library,
LibraryNamespace, LibraryPath, SourceManager, Version,
};
}

And in this file, for example, we import Assembler from assembly directly:

use assembly::{Assembler, Compile, Library};

That is unnecessary. Here we can replace the use assembly::Assembler with a use crate::assembly::Assembler.

So the issue only applies to re-exports. Does that make sense?

Btw. your first commit is not signed. Can you sign it? Or alternatively, opening a new PR is also fine :) Thank you!

@varun-doshi
Copy link
Contributor Author

@PhilippGackstatter I've opened a new PR for miden-objects following your suggestions about using super. Will open one for miden-lib soon.

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