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

[FEATURE] Add a simple string cache #1097

Merged
merged 2 commits into from
Aug 5, 2021
Merged

[FEATURE] Add a simple string cache #1097

merged 2 commits into from
Aug 5, 2021

Conversation

oliverklee
Copy link
Contributor

This class then will later be used to replace the current caching
that is based on nested arrays.

Part of #144.

@oliverklee oliverklee added this to the 6.0.0 milestone Jul 11, 2021
@oliverklee oliverklee self-assigned this Jul 11, 2021
This class then will later be used to replace the current caching
that is based on nested arrays.

Part of #144.
Copy link
Contributor

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

Could the class implement ArrayAccess (instead of the set/get/has methods)? This would also mean fewer changes to the existing code which could continue to get, set and test the cached values using the [] operator and isset.

@oliverklee
Copy link
Contributor Author

As far as I understand it, this would mostly remove the benefit of this class over using a plain array:

  • typing the keys to strings
  • typing the values to strings
  • having more "speaking" method names.

So I'd prefer to now implement ArrayAccess for these reasons.

@JakeQZ
Copy link
Contributor

JakeQZ commented Jul 11, 2021

  • typing the keys to strings
  • typing the values to strings

Yeah, I wondered about that, and can't think of or find a solution. Unlike C++, PHP does not have templates so it's not possible to implment ArrayAccess<string, string>.

Copy link
Contributor

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

Looks generally good, and I can't think of any tests that are missing.

A few minor issues and questions.

phpcs.xml Show resolved Hide resolved
src/Caching/SimpleStringCache.php Outdated Show resolved Hide resolved
src/Caching/SimpleStringCache.php Show resolved Hide resolved
src/Caching/SimpleStringCache.php Show resolved Hide resolved
src/Caching/SimpleStringCache.php Outdated Show resolved Hide resolved
tests/Unit/Caching/SimpleStringCacheTest.php Outdated Show resolved Hide resolved
tests/Unit/Caching/SimpleStringCacheTest.php Outdated Show resolved Hide resolved
tests/Unit/Caching/SimpleStringCacheTest.php Outdated Show resolved Hide resolved
@oliverklee
Copy link
Contributor Author

@JakeQZ I immensely appreciate your detailed reviews! ❤️ On it.

@oliverklee oliverklee requested a review from JakeQZ July 18, 2021 11:18
Copy link
Contributor

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

Lovely. (Sorry for the delay getting back to you, been away quite a but and also busy on other projects.)

src/Caching/SimpleStringCache.php Show resolved Hide resolved
phpcs.xml Show resolved Hide resolved
@JakeQZ JakeQZ merged commit d614400 into main Aug 5, 2021
@JakeQZ JakeQZ deleted the feature/cache branch August 5, 2021 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants