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

refactor(crates): change to workspace with separate crates #66

Merged
merged 14 commits into from
Nov 5, 2023

Conversation

Xenira
Copy link
Contributor

@Xenira Xenira commented Oct 27, 2023

Description of change

Refactored code into separate crates in a workspace as preparation for #54

Note: I needed to do a few code changes to get proper separation. Ill add comments to those.

How has this been tested? (if applicable)

  • Build and executed
  • GH Actions might need to be refactored? ✔️

@Xenira Xenira requested a review from orhun as a code owner October 27, 2023 20:26
crates/daktilo_lib/src/audio.rs Show resolved Hide resolved
crates/daktilo_lib/src/config.rs Show resolved Hide resolved
crates/daktilo_lib/src/embed.rs Show resolved Hide resolved
crates/daktilo_lib/src/lib.rs Show resolved Hide resolved
@Xenira
Copy link
Contributor Author

Xenira commented Oct 27, 2023

msrv does not support workspace inheritance. Added a workaround, that needs to be updated for each crate.

@Xenira
Copy link
Contributor Author

Xenira commented Oct 27, 2023

Not sure why tests are failing. They work locally...

Edit: Found out why. When OUT_DIR is not set the tests skip without error. Changed it to use target dir by default.

@Xenira Xenira force-pushed the workspaces branch 5 times, most recently from 55b1070 to 2500e2a Compare October 28, 2023 10:20
@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2023

Codecov Report

Attention: 70 lines in your changes are missing coverage. Please review.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Files Coverage Δ
crates/daktilo_lib/src/app.rs 0.00% <ø> (ø)
crates/daktilo_lib/src/config.rs 43.20% <100.00%> (ø)
crates/daktilo_lib/src/error.rs 91.66% <ø> (ø)
crates/daktilo_lib/src/lib.rs 0.00% <ø> (ø)
crates/daktilo/src/args.rs 16.12% <0.00%> (ø)
crates/daktilo/src/bin/completions.rs 90.00% <85.71%> (ø)
crates/daktilo/src/bin/mangen.rs 91.30% <85.71%> (ø)
crates/daktilo_lib/src/embed.rs 60.00% <0.00%> (ø)
crates/daktilo_lib/src/audio.rs 0.00% <0.00%> (ø)
crates/daktilo_lib/src/logger.rs 0.00% <0.00%> (ø)
... and 1 more

📢 Thoughts on this report? Let us know!.

@orhun
Copy link
Owner

orhun commented Nov 1, 2023

Sorry, busy days, will try to review this soon.

@Xenira
Copy link
Contributor Author

Xenira commented Nov 1, 2023

No worries, same for me. Take your time :)

Copy link
Owner

@orhun orhun left a comment

Choose a reason for hiding this comment

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

I had some initial comments!

.github/workflows/ci.yml Outdated Show resolved Hide resolved
crates/daktilo/src/lib.rs Outdated Show resolved Hide resolved
crates/daktilo_lib/Cargo.toml Outdated Show resolved Hide resolved
crates/daktilo/Cargo.toml Outdated Show resolved Hide resolved
crates/daktilo_lib/src/embed.rs Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
crates/daktilo/src/bin/completions.rs Show resolved Hide resolved
Xenira added a commit to Xenira/daktilo that referenced this pull request Nov 2, 2023
Xenira added a commit to Xenira/daktilo that referenced this pull request Nov 2, 2023
Xenira added a commit to Xenira/daktilo that referenced this pull request Nov 2, 2023
Xenira added a commit to Xenira/daktilo that referenced this pull request Nov 2, 2023
crates/daktilo_lib/Cargo.toml Outdated Show resolved Hide resolved
@orhun
Copy link
Owner

orhun commented Nov 4, 2023

With a quick check I realized the tracing logs do not appear when I do cargo run, can you check?

I think we need to update the directive in the logger::init

@Xenira
Copy link
Contributor Author

Xenira commented Nov 4, 2023

Fixed it. Problem was, that it was using CARGO_PKG_NAME which was daktilo_lib causing daktilo not to log.

My first approach was to provide different log levels using the verbose flag, which was not the cause of the problem. Decided to keep it in, as it is an improvement over the old version.

Copy link
Owner

@orhun orhun left a comment

Choose a reason for hiding this comment

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

LGTM! (I pushed some commits for polishing - move fast and break things ™️)

@orhun orhun merged commit 5776676 into orhun:main Nov 5, 2023
14 checks passed
@orhun orhun mentioned this pull request Nov 5, 2023
@orhun
Copy link
Owner

orhun commented Nov 5, 2023

FYI release-plz fails to find the embedded assets - I can look into it soon (before the release probably).

message: #[derive(RustEmbed)] folder '/home/runner/work/daktilo/daktilo/target/package/daktilo_lib-0.5.0/../../sounds' does not exist

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.

3 participants