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

Remove dangerous FlagValueDictionary functionality #126

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

KeithBauerANZ
Copy link
Collaborator

@KeithBauerANZ KeithBauerANZ commented Aug 2, 2024

📒 Description

Remove dangerous FlagValueDictionary functionality:

  • It's internally locked, so not safe for it to conform to Collection
  • It's a reference type, so shouldn't conform to Equatable

🔍 Detailed Design

Include any additional information about the design here. At minimum, show any new API:

... FlagValueDictionary {

    // MARK: - Dictionary Access

    /// Returns a copy of the current values in this source
    var allValues: DictionaryType { get }

}

📓 Documentation Plan

I noticed CustomSources.md is outdated with the new FlagValueSource/NonSendableFlagValueSource split, but updating that seems to be a separate task.

🗳 Test Plan

Unit tests.

🧯 Source Impact

  • Removes FlagValueDictionary's conformance to Collection. This is a breaking change. Existing users of this conformance will need to use allValues, but that alone can't replace all uses of the Collection API. I didn't want to make allValues a mutable property though, because that would reintroduce the same kinds of unsafety that the Collection conformance did.

  • Removes FlagValueDictionary's conformance to Equatable. This is a breaking change. Existing users of this conformance will need to check sourceA.id == sourceB.id && sourceA.allValues == sourceB.allValues to get an equivalent check (and also be aware that they may have a race condition since the FVD is Sendable)

✅ Checklist

  • I've added at least one test that validates that my change is working, if appropriate
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

* It's internally locked, so not safe for it to conform to Collection
* It's a reference type, so shouldn't conform to Equatable
@KeithBauerANZ KeithBauerANZ changed the title Remove dangerous FVD functionality Remove dangerous FlagValueDictionary functionality Aug 2, 2024
@KeithBauerANZ KeithBauerANZ added the vexil3 Part of the Vexil 3 alpha/beta development label Aug 2, 2024
@KeithBauerANZ KeithBauerANZ requested a review from bok- August 2, 2024 01:41
Copy link

sonarcloud bot commented Aug 2, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vexil3 Part of the Vexil 3 alpha/beta development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant