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 cache option #52

Merged
merged 6 commits into from
May 15, 2020
Merged

Add cache option #52

merged 6 commits into from
May 15, 2020

Conversation

wfeller
Copy link
Contributor

@wfeller wfeller commented May 10, 2020

Is it useful to support multiple cache drivers like Laravel or would it be enough to simply only use the memory cache?

Solves #51

@iksaku
Copy link
Contributor

iksaku commented May 10, 2020

I think using a Generic Cache adapter could suffice... Lets hear what @sausin thinks about this

@sausin
Copy link
Owner

sausin commented May 11, 2020

Loving it!! I think the generic adapter looks good. We're primarily a laravel package but no harm in making things more generic if it's possible 👍

@wfeller Could you consider some tests for it?

EDIT:- Had misread what you meant. I think memory cache support is sufficient for now. We can consider additional complexity with other drivers if times call for it

@sausin
Copy link
Owner

sausin commented May 11, 2020

Also just noticed, shouldn't the new dependencies be in require instead of require-dev ?

composer.json Outdated Show resolved Hide resolved
@wfeller
Copy link
Contributor Author

wfeller commented May 11, 2020

No, the other dependencies are only required if the users want to use the cache features, in which case they can install them themselves (I'll add a suggest part in composer.json like Laravel does it - see https://github.com/laravel/framework/blob/1dc032a15037330c5065b84ce3937d075a673b5d/composer.json#L130).

I'll update the PR to only support memory cache + tests tonight or tomorrow.

@sausin
Copy link
Owner

sausin commented May 11, 2020

I see, makes sense. We should reflect the same in the config documentation as well 👍

@wfeller
Copy link
Contributor Author

wfeller commented May 12, 2020

@sausin @iksaku Please take a look at the PR and let me know if you think anything should be updated, otherwise I believe it's good to go.

@sausin
Copy link
Owner

sausin commented May 13, 2020

Looks good to me. Your thoughts @iksaku ?

@sausin sausin merged commit b899a8b into sausin:master May 15, 2020
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.

None yet

3 participants