-
Notifications
You must be signed in to change notification settings - Fork 37
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
Consider cancelled tasks as checked #191
Consider cancelled tasks as checked #191
Conversation
Also hope for this feature! |
@@ -10,6 +10,7 @@ export class Todo { | |||
// suffix: '] ' | |||
// body: hello | |||
private static readonly regexp = /^(?<prefix>((> ?)*)?\s*[\-\*][ ]+\[)(?<check>.)(?<suffix>\]\s+)(?<body>.*)$/; | |||
private static readonly checkedStatuses = ['x', '-']; |
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.
To make it customisable as it is, this would have to either
- be passed from the controller somehow, this will cause the setting to be passed around through every function call.
- it should access the global settings from the model. This won't work, as the model does not depend on Obsidian, and when importing
{ SETTINGS } from 'settings'
tests will fail
None of these is an acceptable solution, so I decided to leave this as a hardcoded list.
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 think that's good enough! Thank you!
@uphy any thoughts? |
I'm sorry for the delay in reviewing. I think this is a very good implementation that is both necessary and sufficient. related: #208 @8bitbuddhist Though this implementation will not ignore the IN_PROGRESS status ( |
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! Thanks!!
Fixes #84 (partially, only adds cancelled tasks and it is not customizable).
I tried making it customizable via settings, but I think I'm not following the convention on the different classes, so I think it's better to just ship this PR first and address customisation separately.