Skip to content
This repository has been archived by the owner on Jan 11, 2024. It is now read-only.

move to lib #30

Merged
merged 7 commits into from
Feb 24, 2023
Merged

move to lib #30

merged 7 commits into from
Feb 24, 2023

Conversation

cryptoAtwill
Copy link
Contributor

@cryptoAtwill cryptoAtwill commented Feb 23, 2023

To migrate main.rs to use both main.rs and lib.rs, reasons are as following:

  1. Currently we are using main.rs, but it's empty and it leads to a lot of clippy dead code warning. Moving to lib.rs will resolve these warnings.
  2. When writing integration tests for SubnetManager, it needs to import modules from ipc-agent, hence it's only possible if we change to lib.
  3. Seems doc tests are compiled only with lib, see Doctests don't work in bin targets, non-public items rust-lang/rust#50784, I could be wrong, but somehow cargo test does not trigger the doc tests, with lib it's just easy.

Since there is the chance, I also fixed a few clippy warning, so that we are now warning free!

Copy link
Contributor

@adlrocha adlrocha left a comment

Choose a reason for hiding this comment

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

LGTM. Let's get a 👍 from @hmoniz that he is OK with this approach too and let's get it merged.

Cargo.toml Outdated Show resolved Hide resolved
src/config/mod.rs Outdated Show resolved Hide resolved
src/config/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@hmoniz hmoniz left a comment

Choose a reason for hiding this comment

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

LGTM. Assuming you'll address Akosh's comments.

src/config/mod.rs Outdated Show resolved Hide resolved
src/config/mod.rs Outdated Show resolved Hide resolved
src/config/mod.rs Outdated Show resolved Hide resolved
@cryptoAtwill cryptoAtwill merged commit 52795ff into main Feb 24, 2023
@cryptoAtwill cryptoAtwill deleted the convert_to_lib branch February 24, 2023 11:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants