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

Database events #209

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Database events #209

wants to merge 12 commits into from

Conversation

cmutel
Copy link
Member

@cmutel cmutel commented Nov 12, 2024

No description provided.

@cmutel cmutel marked this pull request as draft November 12, 2024 22:14
@cmutel
Copy link
Member Author

cmutel commented Nov 12, 2024

Draft until tests for database copy and rename are added. These events are not new signals, but should be a combination of existing signals, and should just work™️, but need to be tested first.

You don't have to love this code, BTW 😛. But making it much better would require refactoring...

Too a while because this is a totally different way of saving and interacting with the data, no SQLite here (yet).

Comment on lines 576 to 579
try:
delta = revisions.generate_delta(old, new, operation)
except NoRevisionNeeded:
return
Copy link
Member

Choose a reason for hiding this comment

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

Why do we raise if we then just ignore it? Would it be better for the execution flow to just return None and act here accordingly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Made this change in 3728040, but I don't love it - I find it cleaner to have the functions always have a single return signature. In any case it should be fixed now.

@cmutel cmutel marked this pull request as ready for review November 13, 2024 21:26
@cmutel
Copy link
Member Author

cmutel commented Nov 13, 2024

Ready for review

@cmutel
Copy link
Member Author

cmutel commented Nov 14, 2024

Rebased after #207 was merged.

@cmutel cmutel marked this pull request as draft November 14, 2024 15:42
@cmutel
Copy link
Member Author

cmutel commented Nov 14, 2024

Moved back to draft after some tests are now failing.

@cmutel cmutel marked this pull request as ready for review November 14, 2024 16:13
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.

2 participants