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

Implement undo #520

Closed
wants to merge 9 commits into from
Closed

Implement undo #520

wants to merge 9 commits into from

Conversation

r4ulill0
Copy link
Contributor

@r4ulill0 r4ulill0 commented Apr 7, 2023

The undo functionality does not really resolve recurring todos because that functionality is not implemented yet so there is a test that will fail until that is implemented (I'm planing to solve that myself, but one thing at a time).

@WhyNotHugo tell me if you see something off and sorry for taking that long, recurring todos distracted me a bit 😆

todoman/model.py Outdated Show resolved Hide resolved
@WhyNotHugo
Copy link
Member

I don't fully understand why one tests is failing: https://builds.sr.ht/~whynothugo/job/981248#task-test-262

@r4ulill0
Copy link
Contributor Author

r4ulill0 commented Apr 28, 2023

I don't fully understand why one tests is failing: https://builds.sr.ht/~whynothugo/job/981248#task-test-262

Recurring test doesn't work because there are some recursion fields that are not persisted yet. We have to deal with some issues #485 in order for it to work. I'm planning on taking all of them but one step at a time.

I tried to implement persistence for RELATED-TO but it was a pretty big change and it has its own issue #288.
You can open a new issue to fix this test and add it to #485 if you want. Also we can delete the test until its relevant. I'm fine with either option.

@r4ulill0
Copy link
Contributor Author

r4ulill0 commented May 22, 2023

I have marked "not implemented" the test as the RELATED-TO property needs to be persisted before it can work.

Now arch build passes (Alpine build just exploded and I don't understand why).

@r4ulill0 r4ulill0 requested a review from WhyNotHugo May 22, 2023 16:53
@hrueschwein
Copy link

Is there any progress?..

@r4ulill0
Copy link
Contributor Author

Is there any progress?..

It should be ready to merge. I don't know if @WhyNotHugo has any more things to check.

todoman/cli.py Outdated Show resolved Hide resolved
todoman/model.py Outdated Show resolved Hide resolved
todoman/model.py Outdated Show resolved Hide resolved
tests/test_cli.py Show resolved Hide resolved
Returns the todo that should be deleted.
"""
recurred = None
if self.is_recurring and self.related:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran:

todo new -l test test
todo new 1

The completed todo looks like this:

BEGIN:VCALENDAR
VERSION:2.0
PRODID:io.barrera.todoman
BEGIN:VTODO
COMPLETED:20231118T090119Z
CREATED:20231118T090107Z
DTSTAMP:20231118T090107Z
DUE:20231119T090107Z
LAST-MODIFIED:20231118T090119Z
PERCENT-COMPLETE:100
SEQUENCE:2
STATUS:COMPLETED
SUMMARY:test
UID:[email protected]
END:VTODO
END:VCALENDAR

It doesn't have a recurrence rule any more (that is moved to the next one in the series), so this condition does not match.

If I run todo undo 1, both todos are kept around.

This is not ideal, but I don't think that it is terrible either. The RFC doesn't even cover how a recurring TODO is completed, so there's no correct behavior to any of this either way.

However, this if will never run, so this code is misleading. I think it should be removed (unless I'm missing something here).

@r4ulill0 r4ulill0 closed this Nov 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants