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

Update dependencies and handle the now-async Activitywatch client #40

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

afiorillo
Copy link

@afiorillo afiorillo commented Aug 20, 2024

Hi!

When I tried cloning and building the project, it failed with a few different errors. I am running:

$ rustc -V
rustc 1.80.1 (3f5fd8dd4 2024-08-06)
$ cargo -V
cargo 1.80.1 (376290515 2024-07-16)

The main error seemed to be in a dependency, so I ran cargo update and made fixes according to the interface changes in the ActivityWatch client. Since it's now mostly async I also introduced tokio to provide a runtime.

An IDE extension also automatically ran rustfmt so a number of syntax changes were also introduced -- sorted imports, specified types for literals, etc.


🚀 This description was created by Ellipsis for commit 09e0ce5

Summary:

This PR updates dependencies, introduces tokio for async operations, and refactors src/main.rs to handle the async Activitywatch client.

Key points:

  • Updated Cargo.toml to include tokio with macros and rt-multi-thread features.
  • Modified src/main.rs to use async/await syntax for aw_client_rust::AwClient operations.
  • Introduced tokio runtime in src/main.rs with #[tokio::main] attribute.
  • Updated Cargo.lock to reflect new dependency versions.
  • Applied rustfmt for code formatting, affecting import order and spacing.

Generated with ❤️ by ellipsis.dev

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 09e0ce5 in 31 seconds

More details
  • Looked at 1987 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. src/main.rs:62
  • Draft comment:
    Consider using const instead of static for HEARTBEAT_INTERVAL_MS and HEARTBEAT_INTERVAL_MARGIN_S as their values are known at compile time and do not change.
const HEARTBEAT_INTERVAL_MS: u32 = 5000;
const HEARTBEAT_INTERVAL_MARGIN_S: f64 = (HEARTBEAT_INTERVAL_MS + 1000) as f64 / 1000.0;
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The use of static for HEARTBEAT_INTERVAL_MS and HEARTBEAT_INTERVAL_MARGIN_S is not ideal. These should be const since their values are known at compile time and do not change.
2. src/main.rs:174
  • Draft comment:
    Consider using a logging framework instead of println! for better control over log levels and output.
// Use a logging framework instead of println!
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The println! statements for debugging purposes should be removed or replaced with a logging framework for better control over log levels.

Workflow ID: wflow_X9T5jHEu8DOpNAVZ


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Member

@ErikBjare ErikBjare left a comment

Choose a reason for hiding this comment

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

Not sure if this needed to be made async, as the blocking AwClient is still available, it was just moved (this was poorly done tbh, should not have broken the existing blocking API): https://github.com/ActivityWatch/aw-server-rust/blob/master/aw-client-rust/src/blocking.rs

But looks good to me! I'll leave the final say to someone who actually uses the watcher, like @johan-bjareholt.

@@ -1,39 +1,44 @@
// The generated code will import stuff from wayland_sys
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what this comment was for, could prob be removed.

@ErikBjare
Copy link
Member

I merged #36 which only fixes the compiler errors without switching to tokio/async.

@afiorillo
Copy link
Author

Not sure if this needed to be made async, as the blocking AwClient is still available, it was just moved (this was poorly done tbh, should not have broken the existing blocking API): https://github.com/ActivityWatch/aw-server-rust/blob/master/aw-client-rust/src/blocking.rs

But looks good to me! I'll leave the final say to someone who actually uses the watcher, like @johan-bjareholt.

This is good to know! I tend to agree it's better to keep the changes minimal. I only did these to quickly build the project so I could try it out, and unfortunately I found afterwards that my laptop's Wayland compositor (probably) isn't supported. I don't have enough time now to investigate further, but I thought I'd share the compilation fixes.

Thanks for the super quick reply! Feel free to close this PR since the conflicts may require special attention.

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