Todo sync now handles priorty correctly #203
Conversation
WalkthroughThe changes in this pull request involve updates to three main files: the project file Changes
Assessment against linked issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- Guppi.Console/Guppi.Console.csproj (1 hunks)
- Guppi.Console/Properties/launchSettings.json (1 hunks)
- Guppi.Core/Services/TodoService.cs (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Guppi.Console/Guppi.Console.csproj
🔇 Additional comments (1)
Guppi.Console/Properties/launchSettings.json (1)
5-5: LGTM! Development environment configuration aligned with feature work.The updated command line arguments appropriately reflect the current development focus on todo synchronization, making it easier to test the priority handling fixes. Note that this change only affects the development environment and won't impact production behavior.
| title = title.Substring(3).Trim(); | ||
| } | ||
|
|
||
| string taskStr = $"{priority}{task.Updated.GetRfc3339Date().ToString("yyyy-MM-dd")} {title}"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve task string formatting.
While the priority is now correctly placed before the date, the string construction could be improved.
Consider this more readable format using string interpolation:
-string taskStr = $"{priority}{task.Updated.GetRfc3339Date().ToString("yyyy-MM-dd")} {title}";
+string taskStr = $"{priority} {task.Updated.GetRfc3339Date():yyyy-MM-dd} {title}";This change:
- Adds a space between priority and date for better readability
- Uses a cleaner date format syntax
- Maintains compatibility with todo.txt format
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| string taskStr = $"{priority}{task.Updated.GetRfc3339Date().ToString("yyyy-MM-dd")} {title}"; | |
| string taskStr = $"{priority} {task.Updated.GetRfc3339Date():yyyy-MM-dd} {title}"; |
| // Pull the priority out of the title | ||
| string priority = string.Empty; | ||
| string title = task.Title; | ||
| if (title.Length >= 3 && title[0] == '(' && title[2] == ')' && title[3] == ' ') | ||
| { | ||
| priority = title.Substring(0, 4); | ||
| title = title.Substring(3).Trim(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Enhance priority validation and extraction.
The priority extraction logic could be improved to handle edge cases and validate the priority format more strictly.
Consider applying these improvements:
string priority = string.Empty;
string title = task.Title;
-if (title.Length >= 3 && title[0] == '(' && title[2] == ')' && title[3] == ' ')
+if (title.Length >= 4 && title[0] == '(' && title[2] == ')' && title[3] == ' ')
{
+ char priorityLetter = title[1];
+ if (priorityLetter >= 'A' && priorityLetter <= 'Z')
+ {
priority = title.Substring(0, 4);
- title = title.Substring(3).Trim();
+ title = title.Substring(4).Trim();
+ }
}This change:
- Validates that priority letters are uppercase A-Z
- Uses more accurate substring length (4 instead of 3)
- Properly handles the space after priority
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Pull the priority out of the title | |
| string priority = string.Empty; | |
| string title = task.Title; | |
| if (title.Length >= 3 && title[0] == '(' && title[2] == ')' && title[3] == ' ') | |
| { | |
| priority = title.Substring(0, 4); | |
| title = title.Substring(3).Trim(); | |
| } | |
| // Pull the priority out of the title | |
| string priority = string.Empty; | |
| string title = task.Title; | |
| if (title.Length >= 4 && title[0] == '(' && title[2] == ')' && title[3] == ' ') | |
| { | |
| char priorityLetter = title[1]; | |
| if (priorityLetter >= 'A' && priorityLetter <= 'Z') | |
| { | |
| priority = title.Substring(0, 4); | |
| title = title.Substring(4).Trim(); | |
| } | |
| } |
| // Pull the priority out of the title | ||
| string priority = string.Empty; | ||
| string title = task.Title; | ||
| if (title.Length >= 3 && title[0] == '(' && title[2] == ')' && title[3] == ' ') | ||
| { | ||
| priority = title.Substring(0, 4); | ||
| title = title.Substring(3).Trim(); | ||
| } | ||
|
|
||
| string taskStr = $"{priority}{task.Updated.GetRfc3339Date().ToString("yyyy-MM-dd")} {title}"; |
There was a problem hiding this comment.
💡 Codebase verification
Priority sync implementation is incomplete for local to Google direction
The code review reveals an asymmetric implementation of priority handling:
- When syncing from Google to local (todo.txt), priorities are correctly extracted from the title and preserved in the format
(X) YYYY-MM-DD task - When syncing from local to Google (line 89), the task text is sent directly to Google without preserving the priority format:
var newTask = new Google.Apis.Tasks.v1.Data.Task { Title = task.Text };
This means priorities set in local todo.txt files will be lost when syncing to Google Tasks.
🔗 Analysis chain
Verify priority sync behavior in both directions.
The implementation looks good for handling priorities from Google Tasks to local todo.txt. However, we should verify the behavior in both directions.
Let's check for any existing priority-related tests:
Would you like me to help create test cases to verify:
- Syncing tasks with priorities from local to Google
- Handling malformed priority formats
- Priority preservation during round-trip sync
I'll search for priority handling and test implementations to verify the sync behavior in both directions. The previous search might have been too specific, so let's broaden it.
I need to verify how the task titles are handled in both sync directions to understand the complete priority sync implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for priority-related test cases
rg -l "priority.*test" --type cs
# Check for existing todo.txt test files
fd -e txt "todo.*test"
Length of output: 59
Script:
#!/bin/bash
# Let's try a broader search for priority-related code and tests
rg -i "priority" --type cs -A 3 -B 3
# Also search for task-related test files
fd -e cs "test"
# Look for any existing test classes
ast-grep --pattern 'class $_ { $$$ [TestMethod] $$$ }'
Length of output: 2226
Script:
#!/bin/bash
# Let's look for the complete TodoService implementation to understand the bidirectional sync
ast-grep --pattern 'class TodoService { $$$ }'
# Search for todo.txt related code
rg "todo.txt" --type cs -A 3 -B 3
# Look for task title handling in both directions
rg "Title" --type cs -A 3 -B 3 Guppi.Core/Services/TodoService.cs
Length of output: 3319
Fixes #194
Summary by CodeRabbit
New Features
Improvements
Bug Fixes