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

Improve overload resolution for KnowledgeSource protocols #16

Merged
merged 4 commits into from
Sep 26, 2024

Conversation

Supereg
Copy link
Member

@Supereg Supereg commented Aug 26, 2024

Improve overload resolution for KnowledgeSource protocols

♻️ Current situation & Problem

A SharedRepository typically has multiple overloads for a range of different KnowledgeSource protocols. A knowledge source might provide default values or might be computed. Typically, you only adopt a single knowledge source protocol. However, you cannot prevent users from adopting multiple. This PR improves the overload resolution in these scenarios. All of the specialized KnowledgeSource protocols inherit the base KnowledgeSource protocol. Therefore, Swift resolution will automatically prefer the more specialized type. Problems arise if you accidentally (or on purpose) conform to a computed KnowledgeSource variant and the DefaultProvidingKnowledgeSource. In this case we resolve the conflict and generally prefer the computed knowledge source protocol. We consider a computed knowledge source more specialized than only providing a default value.

⚙️ Release Notes

  • Improve overload resolution for knowledge source protocols in certain situations

📚 Documentation

--

✅ Testing

Added some unit testing to verify overload behavior.

📝 Code of Conduct & Contributing Guidelines

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

Copy link

codecov bot commented Aug 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.06%. Comparing base (17bd0e0) to head (0b16cbd).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #16   +/-   ##
=======================================
  Coverage   83.06%   83.06%           
=======================================
  Files           9        9           
  Lines         301      301           
=======================================
  Hits          250      250           
  Misses         51       51           
Files with missing lines Coverage Δ
...on/SharedRepository/SendableSharedRepository.swift 100.00% <ø> (ø)
...Foundation/SharedRepository/SharedRepository.swift 100.00% <ø> (ø)

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 17bd0e0...0b16cbd. Read the comment docs.

@PSchmiedmayer PSchmiedmayer marked this pull request as ready for review August 27, 2024 05:29
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.

Looks good to me; would be great to add a documentation for this in the method docs and DocC articles to point people to that behavior.

Thank for putting this in place @Supereg!

@Supereg Supereg changed the title Fix/generally prefer computed ks Improve overload resolution and avoid default Value Sep 10, 2024
@Supereg Supereg changed the title Improve overload resolution and avoid default Value Improve overload resolution for KnowledgeSource protocols Sep 10, 2024
@Supereg
Copy link
Member Author

Supereg commented Sep 10, 2024

Looks good to me; would be great to add a documentation for this in the method docs and DocC articles to point people to that behavior.

Thank for putting this in place @Supereg!

I added a PR description for a more detailed explanation. I'm not sure if we need to explicitly document this behavior. This rather is an edge case and this PR resolves overload resolution in these cases. What do you think?

@Supereg Supereg force-pushed the fix/generally-prefer-computed-ks branch from ef7a6e3 to 0b16cbd Compare September 11, 2024 13:22
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.

Sounds great; happy to see this being merged 🚀

@PSchmiedmayer PSchmiedmayer added the enhancement New feature or request label Sep 25, 2024
@Supereg Supereg merged commit 2506285 into main Sep 26, 2024
13 checks passed
@Supereg Supereg deleted the fix/generally-prefer-computed-ks branch September 26, 2024 08:49
Supereg added a commit that referenced this pull request Oct 17, 2024
# Introduce RWLock, ManagedAsynchronousAccess and DataDescriptor

## ♻️ Current situation & Problem
This PR adds the `RWLock`, `RecursiveRWLock` and
`ManagedAsynchronousAccess` infrastructure that was previously
introduced in SpeziBluetooth (see
StanfordSpezi/SpeziBluetooth#45 and
StanfordSpezi/SpeziBluetooth#47).
This changes require the Swift 6 toolchain. Therefore, we increase the
required swift tools version to 6.0.

This PR introduces the final changes for the SpeziFoundation 2.0 release
(assuming #16 is merged beforehand).

## ⚙️ Release Notes 
* Added `RWLock` and `RecursiveRWLock`
* Added `ManagedAsynchronousAccess`
* Only require `sending` closure with `withTimeout` instead of a
`@Sendable` one.
* Add new `DataDescriptor` type.


## 📚 Documentation
Updated the documentation catalog, adding a new "Concurrency" topics
section.


## ✅ Testing
Added unit test for the new components.


## 📝 Code of Conduct & Contributing Guidelines 

By submitting creating this pull request, you agree to follow our [Code
of
Conduct](https://github.com/StanfordSpezi/.github/blob/main/CODE_OF_CONDUCT.md)
and [Contributing
Guidelines](https://github.com/StanfordSpezi/.github/blob/main/CONTRIBUTING.md):
- [x] I agree to follow the [Code of
Conduct](https://github.com/StanfordSpezi/.github/blob/main/CODE_OF_CONDUCT.md)
and [Contributing
Guidelines](https://github.com/StanfordSpezi/.github/blob/main/CONTRIBUTING.md).
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.

2 participants