Skip to content

Conversation

@JaapWijnen
Copy link

@JaapWijnen JaapWijnen commented Jul 20, 2023

This is a first attempt at implementing a DictionaryProtocol protocol as discussed in #293
This is not complete yet and needs discussion in order to improve!
For now it conforms stdlib Dictionary and OrderedDictionary to the newly introduced DictionaryProtocol.
A basic first definition of a MutableDictionaryProtocol has been created but no conformances added yet. (Want to get the base protocol right first and perhaps identify methods that we would want a mutable version to have)
Hoping to get some feedback on the current implementation and will add conformances to the other Dictionary objects next.

Would we also like to introduce a SetProtocol? Should we consider this first since Dictionaries often depend on some type of Set of keys? Or do we keep the keys collection more generic than constraining it to a Set kind of type?

Not all checks are done since this is still in development :) Would love to hear people's thoughts!

Checklist

  • I've read the Contribution Guidelines
  • My contributions are licensed under the Swift license.
  • I've followed the coding style of the rest of the project.
  • I've added tests covering all new code paths my change adds to the project (if appropriate).
  • I've added benchmarks covering new functionality (if appropriate).
  • I've verified that my change does not break any existing tests or introduce unexplained benchmark regressions.
  • I've updated the documentation if necessary.

@JaapWijnen JaapWijnen requested a review from lorentey as a code owner July 20, 2023 14:20
@JaapWijnen
Copy link
Author

Ah forget about the SetProtocol remark. Forgot that the regular Dictionary's Keys is not strictly a Set.

@JaapWijnen
Copy link
Author

@lorentey Would you have a bit of direction for me? I'd love to get this to the finish line but could really use some guidance from you. See also my comments in #293

Copy link

@aluco100 aluco100 left a comment

Choose a reason for hiding this comment

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

LGTM

@JaapWijnen
Copy link
Author

I haven't looked at this in a while and this probably needs more discussion before merging. Would love to work on it more though if anyone has suggestions and/Or feedback!

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