Skip to content

Comments

feat: make journal entries generic#2154

Merged
rakita merged 2 commits intomainfrom
unknown repository
Mar 7, 2025
Merged

feat: make journal entries generic#2154
rakita merged 2 commits intomainfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Mar 6, 2025

This pull requests moves revert functionality into the journal entry trait, and makes it possible to create custom journal entries which are revertible

closes issue:
#2146

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 6, 2025

CodSpeed Performance Report

Merging #2154 will not alter performance

Comparing omer-gen:feat/add-journal-entry-generic (f8a43b5) with main (fd98dc2)

Summary

✅ 8 untouched benchmarks

@ghost ghost marked this pull request as ready for review March 6, 2025 19:28
@ghost
Copy link
Author

ghost commented Mar 6, 2025

@rakita an idea for a future change: it could be easier to maintain if the revert and the change itself are symmetric, meaning that both the original state change and it's revert are handled in the entry, then it could be easier to unitest (checkpoint + do the change and revert, then check state is the same)

@rakita
Copy link
Member

rakita commented Mar 7, 2025

@rakita an idea for a future change: it could be easier to maintain if the revert and the change itself are symmetric, meaning that both the original state change and it's revert are handled in the entry, then it could be easier to unitest (checkpoint + do the change and revert, then check state is the same)

Yeah I assume something like this is going to be implemented in future, that both do the action and know how to revert it. But this is a good step forward

Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

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

lgtm

@rakita rakita merged commit 9ddcad5 into bluealloy:main Mar 7, 2025
28 checks passed
This was referenced Mar 7, 2025
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