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

fix: allow users to provide String and Data values directly #94

Merged
merged 5 commits into from
Dec 9, 2023

Conversation

anitarua
Copy link
Collaborator

@anitarua anitarua commented Dec 8, 2023

Addresses #82

The DataClient still uses the ScalarType enum internally, but the CacheClient provides public-facing methods that accept String and Data values directly. Requires overloading a bunch of functions (for example, set has 4 signatures now), but this was the most straightforward way I could think to implement this at the moment.

Removed the CacheClient protocol and DataClientProtocol extension along the way; I thought it would make more sense for the CacheClient to be its own separate public-facing interface rather than have its type be a union of the ControlClientProtocol and DataClientProtocol.

Open to suggestions / other ideas here though!

@pgautier404
Copy link
Collaborator

There is a lot of repeated code in these functions that I think you can factor out and DRY up quite a bit. For example, the get() methods could be simplified to each just take whatever key came in and wrap it in a ScalarType before calling an internal doGet() method that does all the actual work of validation, error handling, and calling dataClient.get(). I believe the same pattern should hold throughout the overloaded methods.

pgautier404
pgautier404 previously approved these changes Dec 9, 2023
Copy link
Collaborator

@pgautier404 pgautier404 left a comment

Choose a reason for hiding this comment

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


protocol CacheClientProtocol: ControlClientProtocol & DataClientProtocol {}

extension CacheClientProtocol {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

protocol extension was re-added because default parameter values didn't work correctly in tests that expected CacheClientProtocol object instead of just CacheClient

@anitarua anitarua merged commit abdb54c into main Dec 9, 2023
4 checks passed
@anitarua anitarua deleted the scalar-type-data branch December 9, 2023 00:36
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