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.

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

Editing a todo changes its ID #85

Closed
WhyNotHugo opened this issue Feb 9, 2017 · 16 comments · Fixed by #197
Closed

Editing a todo changes its ID #85

WhyNotHugo opened this issue Feb 9, 2017 · 16 comments · Fixed by #197

Comments

@WhyNotHugo
Copy link
Member

Since an edited todo is removed from the cache and re-inserted, it's id changes (but we never have id re-use). Maybe we should avoid this (if possible).

@untitaker
Copy link
Member

I still have mixed feelings about having persistent IDs. I see one disadvantage (that in my case most IDs are guaranteed to have more than one digit), but no advantages.

@WhyNotHugo
Copy link
Member Author

Advantages are that concurrent operations won't change the IDs, which, IMHO, adds a bit of safety:

  • In terminal A run todo, walk away.
  • Open terminal B, run todo, close it.
  • Some time later, notice terminal A still there, and do something like todo done 3 based on what you see on-screen.

It might be a bit uncommon, but I kinda like the determinism this adds.

@untitaker
Copy link
Member

That is fair enough.

@WhyNotHugo
Copy link
Member Author

I'm still willing to discuss it, but I honestly, I generally favour determinism.

@untitaker
Copy link
Member

An easy fix that wouldn't change the nice cache validation flow we have (first expire, then reinsert) would be to directly derive the ID from the UID.

@WhyNotHugo
Copy link
Member Author

I don't think we can derive collision-resistant IDs from UIs due to the amount of digits. Consider that we'd need a two/three digit output for every UID that does not collide (think of it as a hash function).

@untitaker
Copy link
Member

We could throw in lowercase letters into the mix since those are easy to type as well.

Other options would require updating the cache in-place during validation which is somewhat complicated.

@WhyNotHugo
Copy link
Member Author

With letters + numbers (35 symbols), and 80 todos in the db, we'd have a 92% change of collision with two digits, and 7% with three digits. With both lowercase and uppercase letters, this goes down to 1% and 0.5% respectively.

These number are a bit high, because a single collision would make todo fail.
I'm also not convinced that the case-insensitivity of the ids will be obvious.


To get a better understaning though, what causes the dislike for persistent ids? Maybe there's an alternate solution to it it.

@untitaker
Copy link
Member

untitaker commented Feb 13, 2017

I dislike persistent IDs precisely because they fill up the space of possible IDs so much. I usually have a few open tasks where most tasks usually have a single-digit ID. With the new behavior, the tasks that I don't care about (done tasks) and will never address by ID again are assigned a low-digit ID.

@WhyNotHugo
Copy link
Member Author

Ah, I understand, that's really a really valid concern. Let me think if there's another solution. I take it that todo flush is not something you're fond of.

@untitaker
Copy link
Member

I consider todo flush to be a performance workaround but it works.

@WhyNotHugo
Copy link
Member Author

How would you feel about an autoflush setting to auto-delete done tasks? Or do you prefer to keep them around?

@untitaker
Copy link
Member

untitaker commented Feb 13, 2017

I personally have no use for them after I'm done, but we shouldn't assume that the avg user feels about that the same way.

@WhyNotHugo
Copy link
Member Author

Right, I meant, autoflush with default=False.

If this sounds okay to you, I'll prepare a PR, but if you actually want to keep them around, we can go back to brainstorming.

@untitaker
Copy link
Member

Since flush also wipes the cache I imagine that this slows down everything a lot.

@WhyNotHugo
Copy link
Member Author

It does. 😞

Back to the drawing board it is!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants