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.

Sign up for GitHub

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

Todo sync now handles priorty correctly #203

Merged
merged 1 commit into from
Oct 28, 2024
Merged

Todo sync now handles priorty correctly #203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Guppi.Console/Guppi.Console.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
<PackageProjectUrl>https://github.com/rprouse/guppi</PackageProjectUrl>
<RepositoryUrl>https://github.com/rprouse/guppi</RepositoryUrl>
<PackageId>dotnet-guppi</PackageId>
<Version>6.3.1</Version>
<Version>6.3.2</Version>
<PackAsTool>true</PackAsTool>
<ToolCommandName>guppi</ToolCommandName>
<PackageOutputPath>./nupkg</PackageOutputPath>
Expand Down
2 changes: 1 addition & 1 deletion Guppi.Console/Properties/launchSettings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"profiles": {
"Guppi.Console": {
"commandName": "Project",
"commandLineArgs": "bills all"
"commandLineArgs": "todo sync"
}
}
}
11 changes: 10 additions & 1 deletion Guppi.Core/Services/TodoService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,16 @@ public async Task Sync()
.ToList();
foreach (var task in notLocal)
{
string taskStr = $"{task.Updated.GetRfc3339Date().ToString("yyyy-MM-dd")} {task.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();
}

Comment on lines +98 to +106
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
// 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();
}
}

string taskStr = $"{priority}{task.Updated.GetRfc3339Date().ToString("yyyy-MM-dd")} {title}";
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
string taskStr = $"{priority}{task.Updated.GetRfc3339Date().ToString("yyyy-MM-dd")} {title}";
string taskStr = $"{priority} {task.Updated.GetRfc3339Date():yyyy-MM-dd} {title}";

Comment on lines +98 to +107
Copy link

Choose a reason for hiding this comment

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

💡 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:

  1. Syncing tasks with priorities from local to Google
  2. Handling malformed priority formats
  3. 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

if (!string.IsNullOrEmpty(task.Due))
{
taskStr += $" due:{task.Due}";
Expand Down
Loading