search

climech / grit

Multitree-based personal task manager
MIT License
1.67k stars 47 forks source link

Tab characters cause grit import to panic #1

Closed hannah-scott closed 3 years ago

hannah-scott commented 3 years ago

grit import panics if the imported file has tab delimiters. It works if tabs are converted to spaces.

I've attached the error that got thrown, as well as the two files that I used to test it.

import-with-spaces.txt (this works) import-with-tabs.txt (this errors) import-with-tabs.log

The files were made in nano with tabwidth of 4.

hannah-scott commented 3 years ago

Think I've found a fix to this.

In multitree/node.go, line 313:

if origin.ID == 0 || dest.ID == 0 {
    panic("link endpoints must have IDs")
}

Changing 0 to -1 fixes the problem and multitree_test.go still passes. Initialising NewNode with an ID with 1 on line 24 seems to fix things too.

Not sure if this will cause problems elsewhere?

climech commented 3 years ago

You're on the right track, but I suspect there's a lot more wrong going on there -- the truth is I forgot about the command, during the switch from DAGs to multitrees, if I'm not mistaken. The import code doesn't have any tests either!

I'll fix it for the next release and add the tests, probably tomorrow -- I had it working before so it shouldn't take long.

BTW, it doesn't work for spaces either, it's just including them in task names :smile:. I will definitely have to add support for spaces too.

hannah-scott commented 3 years ago

Yeah I realised the spaces thing when I was looking at it today, I just managed to confuse myself!

I have imports working, I think as intended minus the spaces thing. I can submit a pull request if you want, although I haven't written tests.

climech commented 3 years ago

4f226b00128d285bd66c9edf03e8b719eb27f489 probably would cause other problems, and in 3c266a88878495c4cd8e510b52328941eb97bb6d the function shouldn't accept zero, since it's used for user-provided selectors elsewhere -- I shouldn't have used the function there to begin with, and it's fixed now. 6e518bdbf9e261827a0addc7ce1360b157f62eb9 was spot on, thanks for pointing that out!

I ended up reworking the whole thing, it was too confusing. The were also some other issues, like not updating ancestors after insertion.

Props for trying to make sense of that mess! :smile:

hannah-scott commented 3 years ago

Yeah I kind of expected those would break something else, glad to help though.

Really cool project btw :smile: