Skip to content

refactor: create nextOccurrence and nextOccurrenceDate methods in Occurrence #2983

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

Conversation

ilandikov
Copy link
Collaborator

Types of changes

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)

Description

  • Move Occurrence-related behaviour to Occurrence class

Motivation and Context

  • Separate concerns

How has this been tested?

  • unit tests

Checklist

  • My code follows the code style of this project and passes yarn run lint.
  • My change has adequate Unit Test coverage.

Terms

Copy link

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.

(I had a longer version of this message but lost the edit...)

I like where this is going.

Any thoughts on Occurrence.ts though?
And also Occurrence.test.ts?

@ilandikov
Copy link
Collaborator Author

Any thoughts on Occurrence.ts though?

Ofc! Extracted in a later commit/PR

@ilandikov
Copy link
Collaborator Author

And also Occurrence.test.ts?

I did some work on this but not much since changing a line of code in current Occurrenceclass triggers test failure in other tests. However there is definitely room for improving the tests after the refactoring is done.

Maybe we can quickly discuss ways to improve the tests on our next session when all the PRs are merged.

@claremacrae
Copy link
Collaborator

Great - thanks.

@ilandikov
Copy link
Collaborator Author

Just to clarify, I meant that we/I would do it later, after the refactoring is done, since the code being refactored is covered by tests. So would be good if you find some time to review/approve/merge of this PR =)

@claremacrae
Copy link
Collaborator

Understood - am really busy at the moment... But OK, will do it now...

@ilandikov
Copy link
Collaborator Author

Thanks!

@claremacrae
Copy link
Collaborator

Merging un-reviewed - please add 'internal' and 'recurring' labels to this and any recent closed ones that I forgot to do this...

@claremacrae claremacrae merged commit b6370ac into obsidian-tasks-group:main Jul 23, 2024
2 checks passed
@ilandikov ilandikov added type: internal Only regards development or contributing scope: recurrence Anything to do with recurring/repeating tasks labels Jul 23, 2024
@ilandikov
Copy link
Collaborator Author

Merging un-reviewed - please add 'internal' and 'recurring' labels to this and any recent closed ones that I forgot to do this...

Done!

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 type: internal Only regards development or contributing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants