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

fix: Cycle through statuses until TODO is found when marking recurring tasks as DONE #2516

Merged
merged 3 commits into from Dec 22, 2023

Conversation

asokawotulo
Copy link
Contributor

@asokawotulo asokawotulo commented Dec 18, 2023

Description

I looped through the statuses until a TODO status is found. I realize that there's a possibility of getting stuck in an infinite loop, another approach could be to loop for the length of the registered statuses and fallback if no appropriate status is found.

Motivation and Context

Issues #2304 and #2089.

How has this been tested?

I wrote a test in Commands/ToggleDone.test.ts (not sure if this is the appropriate file to add the test to)

Screenshots (if appropriate)

Types of changes

Changes visible to users:

  • Bug fix (prefix: fix - non-breaking change which fixes an issue)
  • New feature (prefix: feat - non-breaking change which adds functionality)
  • Breaking change (prefix: feat!! or fix!! - fix or feature that would cause existing functionality to not work as expected)
  • Documentation (prefix: docs - improvements to any documentation content for users)
  • Sample vault (prefix: vault - improvements to the Tasks-Demo sample vault)
  • Contributing Guidelines (prefix: contrib - any improvements to documentation content for contributors - see Contributing to Tasks)

Internal changes:

  • Refactor (prefix: refactor - non-breaking change which only improves the design or structure of existing code, and making no changes to its external behaviour)
  • Tests (prefix: test - additions and improvements to unit tests and the smoke tests)
  • Infrastructure (prefix: chore - examples include GitHub Actions, issue templates)

Checklist

Terms

Copy link
Collaborator

@claremacrae claremacrae left a comment

Choose a reason for hiding this comment

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

Hi @asokawotulo,

Thank you very much for looking at this.

I've tested it out and suggested improvements that would be needed before merging the code.

If you have any questions, please do ask!

Many thanks.

src/Task.ts Outdated Show resolved Hide resolved
tests/Commands/ToggleDone.test.ts Outdated Show resolved Hide resolved
@claremacrae claremacrae changed the title Cycle through statuses until TODO is found when marking recurring tasks as DONE fix: Cycle through statuses until TODO is found when marking recurring tasks as DONE Dec 19, 2023
@claremacrae claremacrae added the scope: recurrence Anything to do with recurring/repeating tasks label Dec 19, 2023
@asokawotulo
Copy link
Contributor Author

Hey @claremacrae,

I pushed a fix & test for the infinite loop using the example you provided.

Instead of looping until it finds a task with the TODO type, it loops for the length of the registered statuses and if it doesn't find a TODO type it'll just use the DONE statuses next symbol.

@claremacrae
Copy link
Collaborator

Excellent. Thank you.

What happens now if there is more than one TODO task? Will it follow the chain from the current status, or will it find the first status with TODO?

I think it should try to follow the status chain if it can.

@asokawotulo
Copy link
Contributor Author

It should still follow the chain from the current task

@claremacrae
Copy link
Collaborator

It should still follow the chain from the current task

I've tested that in the test vault, and have confirmed that this is true.

However, from reading the code, I'm struggling to see how it works.

Could you please say how the for loop works? Thanks.

@asokawotulo
Copy link
Contributor Author

Actually I noticed a logical bug while writing the comment to explain the code. I've just pushed a commit to fix that.

The code just checks if the next status is of type TODO. If it is not, it moves to the symbol next to it and repeats the same criteria check.

This process continues for the length of the registered status. However, if a matching status is not found, it directly uses the next status after DONE.

Copy link
Collaborator

@claremacrae claremacrae left a comment

Choose a reason for hiding this comment

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

Thank you. I would definitely be OK to merge this now, so please let me know if you don't have time to add the comments suggested below...

Because it's going to need time to update the documentation before I can release the change, I will hold off merging this until I also have time to add more tests and update the docs.

src/Task.ts Show resolved Hide resolved
@claremacrae claremacrae merged commit 503470a into obsidian-tasks-group:main Dec 22, 2023
1 check passed
@claremacrae
Copy link
Collaborator

Fantastic - thank you very much indeed @asokawotulo. 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: recurrence Anything to do with recurring/repeating tasks
Projects
Status: 🎉 Released
Development

Successfully merging this pull request may close these issues.

None yet

2 participants