Fix hidden extension #19
Conversation
xuhcc
commented
Dec 10, 2019
xuhcc
commented
- Fixed rendering of hidden tasks.
- Allowed tasks without text. It's useful for hidden tasks where only projects do matter.
|
@jmhobbs Please take a look at this PR too when you'll have time. |
| if ( matchHidden !== null ) { | ||
| hidden = true; | ||
| } | ||
| return [hidden, line.replace(/h:1/, ''), null]; | ||
| return [hidden, line.replace(hiddenRegex, ''), hidden ? '1' : null]; |
There was a problem hiding this comment.
I'm on board with this, I think. I'm wondering if there is a definition or spec of the hidden extension anywhere, I can't seem to find one. Maybe @bartlibert can chime in since he added it originally.
Additionally, should we refine that regex while we are at it? /\bh:1\b/ would prevent it from matching in the middle of a word.
There was a problem hiding this comment.
There is no definition of the hidden extension as far as I'm aware. This implementation is based on how simpletask does it. I think time++ also supports it, as well as TodoTxtMac and probably some other clients
There was a problem hiding this comment.
Updated the regex in 03f269f
| @@ -232,9 +230,6 @@ function TodoTxtItem ( line, extensions ) { | |||
| line = line.replace( TodoTxt._trim_re, ''); | |||
|
|
|||
| this.text = line; | |||
|
|
|||
| // If we have an empty task, not much point in creating an object. | |||
| if( "" === this.text ) { throw new Exception( "Empty Task" ); } | |||
There was a problem hiding this comment.
I'm not sure I'm comfortable changing this without at least a minor version bump, as it breaks the many years old contract of this function.
That said, if there is a good argument to change this, I'd be game for rolling it into a v0.9.0.
There was a problem hiding this comment.
Empty tasks could be useful when combined with "hidden" tag. For example, one can create a task like this:
h:1 +project1 +project2 +project3 +project4
to define a list of projects so the app can do autocompletion for them.
I can submit this change in a separate pull request if that would make sense.
There was a problem hiding this comment.
Interesting, that is a viable case. I'd prefer to make this change in a minor bump, would you mind pulling it from this PR and then re-submitting once this one is merged?
There was a problem hiding this comment.
I went ahead and did that so I can merge this tonight. I'll start another branch and cherry pick in ee936309de53b0b8149cdcdce0897e261c8f39ff
| it( "should not parse it", function () { | ||
| var item; | ||
| for( i in invalid ) { | ||
| item = new TodoTxtItem( invalid[i].raw, [ new HiddenExtension() ] ); | ||
| expect( item.hidden ).toEqual( false ); | ||
| expect( item.due ).toBeUndefined(); |
There was a problem hiding this comment.
I'm unclear why this test changed 🤔
There was a problem hiding this comment.
The invalid array contains invalid entries for due extension. It doesn't contain entries with "hidden" tag.
There was a problem hiding this comment.
I should have replaced HiddenExtension with DueExtension as well because this file contains tests for due extension.
Fixed this in acde65f
| @@ -54,7 +54,7 @@ describe( "TodoTxtItem with multiple extensions", function() { | |||
| } ); | |||
|
|
|||
| it( "should be hidden", function() { | |||
| expect( item.hidden ).toEqual( target.hidden ); | |||
| expect( item.h ).toEqual( target.hidden ); | |||
There was a problem hiding this comment.
I feel like having the property named hidden is more descriptive. The property has a specific meaning, so shortening it to match the extension tag feels off to me.
There was a problem hiding this comment.
The name of the property is the same as extension's name. I changed it to h because it is also used to render the todo.txt item as string:
https://github.com/jmhobbs/jsTodoTxt/blob/develop/jsTodoTxt.js#L138-L141
If we set the extension name to hidden it would render as hidden:1 which is incorrect.
In theory we can add a separate property to TodoTxtExtension like tag and use it during parsing/rendering instead of name.
There was a problem hiding this comment.
I'm sold here too, thanks for walking me through this, it's been a while since I've been around this code.
