timrogers / litra-rs

💡 Control your Logitech Litra light from the command line
MIT License
35 stars 4 forks source link

API Refactoring #21

Closed Holzhaus closed 8 months ago

Holzhaus commented 8 months ago

This is a rather big API change, but it has several advantages: Instead of using free functions, the API becomes more straightforward to use. Also, we don't query all device properties when enumerating the connected devices.

Furthermore, all serde-related stuff is moved to main.rs as library user may not need them. The last commit may need some discussion, because now the cli feature flag needs to be passed explicitly to build the binary target (cargo run --features=cli). An alternative would be to use a cargo workspace and move the binary to a dedicated crate.

timrogers commented 8 months ago

The last commit may need some discussion, because now the cli feature flag needs to be passed explicitly to build the binary target (cargo run --features=cli). An alternative would be to use a cargo workspace and move the binary to a dedicated crate.

Where is this part configuration? I was expecting to see the "feature declaration" in the code, but I can't see anything like that.

Holzhaus commented 8 months ago

The last commit may need some discussion, because now the cli feature flag needs to be passed explicitly to build the binary target (cargo run --features=cli). An alternative would be to use a cargo workspace and move the binary to a dedicated crate.

Where is this part configuration? I was expecting to see the "feature declaration" in the code, but I can't see anything like that.

sorry, i forgot to push that commit :D Here it is: https://github.com/timrogers/litra-rs/pull/21/commits/42260cd3f339e039a4861ad2f85d3bf194e1e96e

timrogers commented 8 months ago

@Holzhaus Is this done from your perspective?

Does it make sense for me to update the CI workflows to automatically include the "cli" feature?

Holzhaus commented 8 months ago

@Holzhaus Is this done from your perspective?

Yes. But there are still some open review comments (e.g., https://github.com/timrogers/litra-rs/pull/21#discussion_r1491792267). I'd appreciate your feedback unless you consider them resolved.

Does it make sense for me to update the CI workflows to automatically include the "cli" feature?

In https://github.com/timrogers/litra-rs/pull/21/commits/374e8f8855e990e50068f574b01ce6a0d3e24e06 I configued the cli feature as default feature. That way, litra can be build as before (without specifying a feature explicitly). For library usage, this will require default-features = false in the dependency declaration to skip the uneeded depenencies like serde and so on.

Holzhaus commented 8 months ago

I have another small patch in the pipeline to move the range checks into lib.rs instead of main.rs. Will push it soon.

timrogers commented 8 months ago

Just re-tag me for review when you're ready! 😊 Thanks for your hard work on this.

Holzhaus commented 8 months ago

This is ready now (once the checks pass). This has grown a bit, but it now has proper error handling for the entire main.rs and there are no unwrap() calls anymore (except for the two that can never fail due to the surrounding match statement). Hence, the CLI tool should never panic (unless some dependency panics).