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

feat:non-path-discovery for modules #59

Merged
merged 1 commit into from
Jan 19, 2025

Conversation

0xbrayo
Copy link
Member

@0xbrayo 0xbrayo commented Jan 19, 2025

Users can set their own discovery_path for modules.

#40


Important

Add custom discovery_path for module discovery, integrated into Defaults and get_modules_in_path() for Unix and Windows.

  • Behavior:
    • Users can now set a custom discovery_path for modules in Defaults in lib.rs.
    • The discovery_path is added to the system PATH for module discovery in get_modules_in_path() in manager.rs.
  • Defaults:
    • Added discovery_path field to Defaults struct in lib.rs.
    • Default discovery_path set to ~/aw-modules on Unix and C:\Users\<username>\aw-modules on Windows.
  • Module Discovery:
    • get_modules_in_path() in manager.rs now includes discovery_path in the search paths for modules on both Unix and Windows.

This description was created by Ellipsis for a505652. It will automatically update as commits are pushed.

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 a505652 in 14 seconds

More details
  • Looked at 154 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. src-tauri/src/manager.rs:330
  • Draft comment:
    Consider refactoring the code that adds the discovery path to the PATH variable into a helper function to avoid duplication. This is applicable in both the Unix and Windows implementations of get_modules_in_path.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code for adding the discovery path to the PATH variable is duplicated for both Unix and Windows. This can be refactored into a helper function to avoid code duplication and improve maintainability.
2. src-tauri/src/manager.rs:347
  • Draft comment:
    Using unwrap_or_default when joining paths can hide potential errors. Consider handling the error explicitly to understand why joining paths might fail.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The unwrap_or_default usage when joining paths can hide potential errors. It's better to handle the error explicitly to understand why joining paths might fail.
3. src-tauri/src/manager.rs:391
  • Draft comment:
    Using unwrap_or_default when joining paths can hide potential errors. Consider handling the error explicitly to understand why joining paths might fail. This is also applicable in the Unix implementation.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The same issue of using unwrap_or_default when joining paths is present in the Windows implementation as well. It should be addressed similarly.
4. src-tauri/src/lib.rs:197
  • Draft comment:
    Using unwrap_or_default when getting the discovery path can hide potential errors. Consider handling the error explicitly to understand why getting the path might fail.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The unwrap_or_default usage when getting the discovery path can hide potential errors. It's better to handle the error explicitly to understand why getting the path might fail.

Workflow ID: wflow_wBhgZPqLwqpEIvvk


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

@0xbrayo 0xbrayo force-pushed the non-path-discovery branch from a505652 to 15c65ac Compare January 19, 2025 09:52
@0xbrayo
Copy link
Member Author

0xbrayo commented Jan 19, 2025

@ErikBjare any suggestions about this?

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.

Yes this looks good to me :)


// Add discovery path if not already in PATH
if !paths.contains(&config.defaults.discovery_path) {
paths.push(config.defaults.discovery_path.to_owned());
Copy link
Member

Choose a reason for hiding this comment

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

This should be pushed to the front of the list so that it has priority (following PATH-lookup behavior).

@@ -330,29 +330,42 @@ fn start_module_thread(name: String, custom_args: Option<Vec<String>>, tx: Sende
#[cfg(unix)]
fn get_modules_in_path() -> BTreeSet<String> {
let excluded = ["awk", "aw-tauri", "aw-client", "aw-cli"];
Copy link
Member

Choose a reason for hiding this comment

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

Add aw-qt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do!

@0xbrayo 0xbrayo force-pushed the non-path-discovery branch from 15c65ac to 6a709ce Compare January 19, 2025 19:01
@0xbrayo 0xbrayo merged commit fe4cbe9 into ActivityWatch:master Jan 19, 2025
5 checks passed
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