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

Updating any percentage/fraction cookies in parent of TODO/DONE items #545

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

functionreturnfunction
Copy link

This is intended to address issue #525; apologies for applying business logic in the repository layer but I couldn't find any other spots to pull this off which wouldn't have required a bunch of repetitive changes (such as across the UseCases). I'm certainly open to suggestion on reworking if this is in violation of standards/practices.

@functionreturnfunction functionreturnfunction marked this pull request as ready for review March 9, 2025 22:40
@amberin
Copy link
Member

amberin commented Mar 10, 2025

Awesome! I'll need to look closer to refresh my memory, but I believe you are following the existing practices by putting this in data.DataRepository. I'm not really experienced enough to make judgments on this, but I think it's a bit of a "god class". It has a huge number of responsibilities and there is a big "split this up" TODO at the top. So I'm hoping someone will come along and suggest some refactoring.

Copy link
Member

@amberin amberin left a comment

Choose a reason for hiding this comment

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

Nice work with the tests. This all looks good to me! I haven't tried it out myself, but it looks pretty straightforward and the tests are clear.

@alensiljak
Copy link
Contributor

Thanks for contributing!

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