Skip to content

Conversation

@activecoder10
Copy link
Contributor

No description provided.

@activecoder10 activecoder10 requested a review from xermicus May 9, 2025 12:57
@activecoder10 activecoder10 mentioned this pull request May 9, 2025
Copy link
Member

@xermicus xermicus left a comment

Choose a reason for hiding this comment

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

Looking good! Can you please also add cargo machete? I like how it helps vetting the workspace.

Comment on lines 63 to 65

- name: Test CLI
run: make test-cli
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- name: Test CLI
run: make test-cli

There is no test-cli target in the Makefile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have remove it on an old revision and forgot to clean it from the yml file also. I will do it in the next revision.

Makefile Outdated
clippy:
cargo clippy --all-features --workspace -- --deny warnings

test-workspace:
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Since there is only one could be shortened to just make test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will do

Makefile Outdated
@@ -0,0 +1,10 @@
.PHONY: format clippy test-workspace test-cli
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.PHONY: format clippy test-workspace test-cli
.PHONY: format clippy test-workspace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have remove it on an old revision and forgot to clean it from the yml file also. I will do it in the next revision.

serde_json = { workspace = true }
temp-dir = { workspace = true }

[package.metadata.cargo-machete]
Copy link
Member

Choose a reason for hiding this comment

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

Instead of allowing exceptions please just remove them from the dependencies above. Unless they are actually false positives which I doubt :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking the other way around. Since the project is at the initial phase and there are a lot of things to develop, I didn't want to remove some "unused" dependencies and add them back in the next iteration

Copy link
Member

@xermicus xermicus May 12, 2025

Choose a reason for hiding this comment

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

I see where you are coming from. But please remove them and remove the exception unless its a false positve. It's the sane for everything else too: We have cargo fmt pass, cargo clippy pass, cargo test pass without any unnecessary exceptions too, so why not start with a passing machete. It's not big deal to add and remove dependencies on the go.

@activecoder10 activecoder10 merged commit ae1174f into main May 12, 2025
3 checks passed
@xermicus xermicus deleted the lv-basic-ci-workflow branch May 12, 2025 10:00
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