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.

Sign up for GitHub

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

Jump to bottom

fix: Removes flag resolving from Confidence child instances #149

Merged
merged 1 commit into from
Jul 2, 2024
Merged

fix: Removes flag resolving from Confidence child instances #149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion ConfidenceDemoApp/ConfidenceDemoApp/ConfidenceDemoApp.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ struct ConfidenceDemoApp: App {
let secret = ProcessInfo.processInfo.environment["CLIENT_SECRET"] ?? ""
let confidence = Confidence.Builder(clientSecret: secret, loggerLevel: .TRACE)
.withContext(initialContext: ["targeting_key": ConfidenceValue(string: UUID.init().uuidString)])
.withRegion(region: .europe)
fabriziodemaria marked this conversation as resolved.
Show resolved Hide resolved
.build()

let status = Status()
Expand Down
5 changes: 1 addition & 4 deletions Sources/Confidence/Confidence.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,10 +262,7 @@ public class Confidence: ConfidenceEventSender {
}
}

/**
Sets the initial Context.
*/
public func withContext(_ context: ConfidenceStruct) -> Self {
public func withContext(_ context: ConfidenceStruct) -> ConfidenceEventSender {
return Self.init(
clientSecret: clientSecret,
region: region,
Expand Down
3 changes: 3 additions & 0 deletions Sources/Confidence/ConfidenceContextProvider.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,8 @@ import Foundation
A Contextual implementer returns the current context
*/
public protocol ConfidenceContextProvider {
/**
Returns the current context, including the ancestors' context data
*/
func getContext() -> ConfidenceStruct
}
15 changes: 14 additions & 1 deletion Sources/Confidence/ConfidenceEventSender.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import Foundation
/**
Sends events to Confidence. Contextual data is appended to each event
*/
public protocol ConfidenceEventSender: Contextual {
public protocol ConfidenceEventSender: ConfidenceContextProvider {
fabriziodemaria marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Would removing Contextual not be a breaking change? I am not sure whether it's necessary to remove 🤔

Copy link
Member Author

@fabriziodemaria fabriziodemaria Jul 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Answering here for @vahidlazio as well: the problem I found with Contextual is that withContext returns a Self, and if it's Confidence implementing such protocol the return type can only be Confidence (while we want ConfidenceEventSender instead)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that removing Contextual is a breaking change, since final users never operate with such type directly: they either have a Confidence instance or a ConfidenceEventSender instance

Copy link
Contributor

Choose a reason for hiding this comment

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

I think my issue with this is that removing Contextual then means this structure doesn't align with Android/JS anymore.
Right, so since Confidence doesn't confirm to EventSender, it won't be able to return one when calling withContext. I think that makes sense 👍 We should document this somewhere, but that's not necessarily part of this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

I think my issue with this is that removing Contextual then means this structure doesn't align with Android/JS anymore.

True, I am also not very about that. But as long as this is internals, it's less of a problem and we can always change it without breaking backwards compat with final users

Right, so since Confidence doesn't confirm to EventSender, it won't be able to return one when calling withContext.

Even if it would conform to EventSender, it seems like the return type Self only accepts the type of the implementing class Confidence

/**
Upon return, the event has been correctly stored and will be emitted to the backend
according to the configured flushing logic
Expand All @@ -18,4 +18,17 @@ public protocol ConfidenceEventSender: Contextual {
Schedule a manual flush of the event data currently stored on disk
*/
func flush()
/**
Adds/override entry to local context data
*/
func putContext(key: String, value: ConfidenceValue)
/**
Removes entry from localcontext data
It hides entries with this key from parents' data (without modifying parents' data)
*/
func removeKey(key: String)
/**
Creates a child event sender instance that maintains access to its parent's data
*/
func withContext(_ context: ConfidenceStruct) -> ConfidenceEventSender
}
22 changes: 0 additions & 22 deletions Sources/Confidence/Contextual.swift
View file
Open in desktop

This file was deleted.

Loading