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 Storage Policy for Computed KnowledgeSources #75

Merged
merged 2 commits into from
Aug 9, 2023

Conversation

Supereg
Copy link
Member

@Supereg Supereg commented Jul 20, 2023

Add Storage Policy for Computed KnowledgeSources

♻️ Current situation & Problem

Currently, all instances of ComputedKnowledgeSource are getting cached. Meaning, the result of the compute method is stored in the SharedRepository and each retrieval will first check if there is a value present before calling compute.
This reduces the control a ComputedKnowledgeSource has on how its value is dynamically computed.
This also has the primary drawback that the respective subscript has to be mutating get.

💡 Proposed solution

This PR introduces the concept of a StoragePolicy for ComputedKnowledgeSources. By default it is set to Store but may be adjusted to AlwaysCompute. Respective subscripts were added to provide the expected implementation.

⚙️ Release Notes

  • Addes Storage Policy for Computed KnowledgeSources

➕ Additional Information

Note: This PR is currently still based of the 0.6.0 release.

Related PRs

Testing

Additionally test vectors were added.

Reviewer Nudging

Look the the newly defined protocols and protocol extensions, as well as the updated SharedRepository implementation.

Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Merging #75 (885d706) into main (c98b2e5) will increase coverage by 2.02%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #75      +/-   ##
==========================================
+ Coverage   95.50%   97.52%   +2.02%     
==========================================
  Files          34       34              
  Lines         600      604       +4     
==========================================
+ Hits          573      589      +16     
+ Misses         27       15      -12     
Files Changed Coverage Δ
Sources/Spezi/Spezi/SpeziLogger.swift 100.00% <ø> (ø)
...edRepository/KnowledgeSource/KnowledgeSource.swift 100.00% <100.00%> (ø)
...rces/Spezi/SharedRepository/SharedRepository.swift 100.00% <100.00%> (+37.50%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c98b2e5...885d706. Read the comment docs.

@Supereg Supereg force-pushed the feature/computed-knowledge-source-storage-policy branch from b7510c3 to 885d706 Compare August 8, 2023 15:51
@Supereg Supereg marked this pull request as ready for review August 8, 2023 15:51
Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you @Supereg, looks great!

@Supereg Supereg merged commit 9ad506d into main Aug 9, 2023
9 checks passed
@Supereg Supereg deleted the feature/computed-knowledge-source-storage-policy branch August 9, 2023 21:27
@PSchmiedmayer PSchmiedmayer added the enhancement New feature or request label Aug 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants