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

Event.persisted? is potentially misleading #235

Merged
merged 3 commits into from
Dec 20, 2023
Merged

Event.persisted? is potentially misleading #235

merged 3 commits into from
Dec 20, 2023

Conversation

andyjdavis
Copy link
Contributor

re #180

Event.persisted? actually just checks if the event has an ID. That behaviour may be surprising. I'm not sure what the best solution is currently however I have written a small test that clearly demonstrates the issue.

@domon-envato
Copy link

Event.persisted? actually just checks if the event has an ID.

How is this method used?

I quickly searched the term persisted in event_sourcery and event_sourcery-postgres but could not find any usage of this method.

Can we potentially remove this method?

@andyjdavis
Copy link
Contributor Author

I am certainly open to simply removing the persisted? method. It seems rather low value with event_sourcery users being able to replicate its functionality very easily

# Is this event persisted?
    def persisted?
      !id.nil?
    end

I will repurpose this PR to remove the event.persisted? method.

Simultaneously I plan to bump the version to 1.0.0 as the removal of persisted? is a breaking change.

@andyjdavis andyjdavis changed the title Adding test to demonstrate that Event.persisted? is potentially misleading Event.persisted? is potentially misleading Nov 28, 2023
@andyjdavis
Copy link
Contributor Author

andyjdavis commented Nov 28, 2023

I have updated this PR to just remove Event.persisted?

I needed to bump the version and, barring any objections, I suggest that we go to 1.0.0. Removing Event.persisted? is a small changed but is a breaking change. Alternatively, it could just go to 0.25.0

I have also updated the readme to have a development status similar to the readme in https://github.com/envato/rack-ecg/ and removed the text about this repo undergoing rapid development as that isn't accurate.

Copy link

@domon-envato domon-envato left a comment

Choose a reason for hiding this comment

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

LGTM

@andyjdavis andyjdavis merged commit 0b21aea into main Dec 20, 2023
10 checks passed
@andyjdavis andyjdavis deleted the EventPersisted branch December 20, 2023 04:49
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