-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: support dash prefix in parseMarkdownChecklist for todo lists #8055
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
Conversation
- Updated regex pattern to support optional dash prefix (e.g., "- [ ] Task") - Added comprehensive test coverage for both formats - Fixes issue where todo lists with dash prefixes were not being parsed correctly Fixes #8054
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed my own code and surprisingly didn't hate it this time.
| for (const line of lines) { | ||
| const match = line.match(/^\[\s*([ xX\-~])\s*\]\s+(.+)$/) | ||
| // Support both "[ ] Task" and "- [ ] Task" formats | ||
| const match = line.match(/^(?:-\s*)?\[\s*([ xX\-~])\s*\]\s+(.+)$/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex pattern (?:-\s*)? elegantly makes the dash prefix optional while maintaining backward compatibility. This is a clean solution that addresses the issue without overcomplicating the implementation.
| const md2 = `- [ ] Task 1` | ||
| const result1 = parseMarkdownChecklist(md1) | ||
| const result2 = parseMarkdownChecklist(md2) | ||
| expect(result1[0].id).toBe(result2[0].id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent test coverage! The 21 test cases thoroughly validate both formats (with and without dash prefix) and cover important edge cases. The ID generation consistency tests at lines 235-240 are particularly valuable for ensuring the same todo item generates the same ID regardless of format.
|
It has been tested and works as expected |
daniel-lxs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thank you @NaccOll!
Description
This PR fixes issue #8054 where the
parseMarkdownChecklistmethod doesn't support checklists with the '-' prefix (e.g.,- [ ] Task), even though markdown supports this syntax.Problem
When users activate the to-do feature, some models (particularly those with poor instruction-following capabilities) may return a to-do list with a dash prefix:
Previously, this would result in an empty todo list being displayed.
Solution
Updated the regex pattern in
parseMarkdownChecklistto support an optional dash prefix:/^\[\s*([ xX\-~])\s*\]\s+(.+)$//^(?:-\s*)?\[\s*([ xX\-~])\s*\]\s+(.+)$/The
(?:-\s*)?pattern makes the dash prefix optional, supporting both formats while maintaining backward compatibility.Testing
Related Issue
Fixes #8054
Type of Change
Checklist
Important
Updated
parseMarkdownChecklistto support dash-prefixed checklists and added comprehensive tests for various formats and edge cases.parseMarkdownChecklistinupdateTodoListTool.tsto support dash-prefixed checklists by changing regex from/^\[\s*([ xX\-~])\s*\]\s+(.+)$/to/^(?:-\s*)?\[\s*([ xX\-~])\s*\]\s+(.+)$/.updateTodoListTool.spec.tscovering standard, dash-prefixed, mixed formats, and edge cases.This description was created by
for 31bbe9d. You can customize this summary. It will automatically update as commits are pushed.