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

Add reload method #491

Merged
merged 4 commits into from
Oct 26, 2023
Merged

Conversation

stufro
Copy link
Contributor

@stufro stufro commented Oct 16, 2023

Resolves #485

@crimson-knight
Copy link
Member

This is basically an alias of find!, and the test example shows that it's meant to be used when you modify the record outside of the object. Other than the convenience of using .reload instead of Model.find!(), what's the benefit of adding this method?

Do you have a use case?

@stufro
Copy link
Contributor Author

stufro commented Oct 16, 2023

We have 2 use cases for it.

The first is reloading a record in specs:

person = Person.create
PersonUpdater.run
person.reload.field.should eq "new value"

The second is in a distributed system when running more than 1 instance of the same service, you might want to reload the record in case another instance has updated the db record since it was loaded into memory.

So yeah I agree it is just a convenience method, happy to go with your judgment 😊

@a-alhusaini
Copy link
Contributor

Might I suggest renaming it to sync instead of reload?

Or maybe resync.. since you're syncronizing with the database.. reload is an ok term but it is more imperative than declarative..

I think resync/sync are better names but I think we can come up with something better if we thought about it.

@crimson-knight
Copy link
Member

I think this would be helpful in the test suite, so let's wrap this in a macro that checks for the Spec module before it's included.

Can you also add the documentation to the code comments? Now that we have the crystaldoc.info I want to ensure we're writing docs that the community at large can consume in as many places as possible.

@stufro stufro force-pushed the 485-add-reload-method branch from 80af7bd to 72b01d4 Compare October 17, 2023 14:31
@stufro stufro force-pushed the 485-add-reload-method branch from 6935e64 to b1d2e5d Compare October 17, 2023 15:00
@stufro
Copy link
Contributor Author

stufro commented Oct 17, 2023

Sure thing all done. 1 questions about the docs, when running crystal docs the reload method doesn't appear I'm assuming because the condition in the macro isn't met. Do you know a good way to deal with this?

src/granite/querying.cr Show resolved Hide resolved
src/granite/querying.cr Show resolved Hide resolved
@crimson-knight crimson-knight merged commit faa7ea2 into amberframework:master Oct 26, 2023
11 of 13 checks passed
@stufro stufro deleted the 485-add-reload-method branch October 28, 2023 08:58
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.

Add reload method
3 participants