feat(sync):daemon support for advanced sync #528
Conversation
|
Should we make the default, syncing all buckets? |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 6241a4a in 2 minutes and 46 seconds
More details
- Looked at
163lines of code in2files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. aw-sync/README.md:25
- Draft comment:
Clarify the expected format for --start-date (YYYY-MM-DD) and that --buckets must be comma‐separated. This will help users avoid format mistakes. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, providing guidance on how to use certain command-line options. It doesn't suggest a code change or ask for clarification on the code itself. It violates the rule against making purely informative comments.
2. aw-sync/src/main.rs:163
- Draft comment:
The comment states the default subcommand is 'Sync', but the code defaults to 'Daemon'. Update the comment to match the implementation. - Reason this comment was not posted:
Comment was on unchanged code.
3. aw-sync/README.md:25
- Draft comment:
Consider adding an example for the--sync-dboption usage (e.g. a valid absolute path) for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While adding an example path could be helpful, the current description is already clear that it's for specifying a database file within the sync directory. The sync directory location is already explained elsewhere in the README (default ~/ActivityWatchSync). Adding an absolute path example could actually be misleading since it should be relative to the sync directory.
The comment might have a point that concrete examples help users understand options better. The current description could be unclear about what constitutes a valid database filename.
However, the existing description is sufficient given that it's clear the file should be in the sync directory, and database filenames are a basic concept. An absolute path example could mislead users about where the file should be located.
The comment should be deleted as the current documentation is clear enough and adding an absolute path example could cause confusion about the expected file location.
4. aw-sync/src/main.rs:297
- Draft comment:
The termination signal (ctrlc) handling logic is duplicated in both daemon() and daemon_advanced(). Consider refactoring this into a helper to reduce repetition. - Reason this comment was not posted:
Marked as duplicate.
5. aw-sync/src/main.rs:218
- Draft comment:
The sync_db path validation logic is repeated both in the SyncAdvanced branch and in daemon_advanced(). Consider extracting this validation into a helper function for maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. aw-sync/src/main.rs:265
- Draft comment:
The hard-coded 300-second timeout is used in multiple places. Consider extracting it into a named constant for clarity and easier modifications. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. aw-sync/README.md:82
- Draft comment:
There's an inconsistency in the casing of 'GDrive'. In line 6 it appears as 'GDrive' and in line 82 it's written as 'Gdrive'. It would be best to standardize the naming. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. aw-sync/src/main.rs:241
- Draft comment:
Typographical error: 'wiggleroom' should be corrected to 'wiggle room' for clarity in the comment. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_CjHolVl5LPjcTWIQ
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| ) -> Result<(), Box<dyn Error>> { | ||
| let (tx, rx) = channel(); | ||
|
|
||
| ctrlc::set_handler(move || { |
There was a problem hiding this comment.
The ctrlc handler setup code is duplicated in both daemon() and daemon_advanced(). Consider refactoring this common logic into a helper function to reduce duplication.
| if let Some(db_path) = &sync_db { | ||
| info!("Using sync db: {}", &db_path.display()); | ||
|
|
||
| if !db_path.is_absolute() { |
There was a problem hiding this comment.
The sync_db path validation logic is duplicated in both SyncAdvanced and daemon_advanced. Consider extracting this into a separate helper function to improve maintainability.
There was a problem hiding this comment.
I don't think it's worth atp, never going to reuse/rewrite any other time.
aw-sync/README.md
Outdated
| aw-sync | ||
|
|
||
| # Advanced sync daemon with custom options | ||
| aw-sync daemon --advanced --buckets "aw-watcher-window,aw-watcher-afk" --start-date "2024-01-01" |
There was a problem hiding this comment.
The --advanced flag seems a bit redundant.
Seems like aw-sync daemon --buckets "aw-watcher-window,aw-watcher-afk" would be a valid command, but will silently not respect the cli params without --advanced.
There was a problem hiding this comment.
I had misread the comment previously. I guess the --advanced is redundant, can be inferred whenever there extra flags.
0xbrayo
force-pushed
the
polish-sync
branch
3 times, most recently
from
0184b97 to
c2380ec
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #528 +/- ##
==========================================
- Coverage 70.81% 69.75% -1.07%
==========================================
Files 51 51
Lines 2916 2959 +43
==========================================
- Hits 2065 2064 -1
- Misses 851 895 +44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
aw-sync/src/main.rs
Outdated
| /// Use advanced sync mode | ||
| /// (automatically enabled when any advanced options are provided) | ||
| #[clap(long)] | ||
| advanced: bool, |
There was a problem hiding this comment.
Necessary for syncing all buckets by default, though I can remove it and use something like --buckets * as an alternative or just sync all buckets by default, resorting to syncing specific buckets when it is specified.
0xbrayo
force-pushed
the
polish-sync
branch
2 times, most recently
from
0ba4d07 to
7c6dd44
Compare
|
Sync all buckets by default now, accepts |
|
Yeah I think it's unnecessary, using |
Important
Adds advanced sync options to
aw-syncdaemon, including bucket selection, start date, and database file specification, with updated CLI and documentation.aw-syncdaemon, allowing specification ofbuckets,start_date, andsync_db.daemon_advanced()inmain.rsfor handling advanced sync logic.Commands::Daemonfor--advanced,--buckets,--start-date, and--sync-db.README.mdwith examples and descriptions of new options.main.rsto reflect completed tasks.This description was created by
for 6241a4a. It will automatically update as commits are pushed.