-
Notifications
You must be signed in to change notification settings - Fork 20
feat: Added ODPManager implementation #322
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
Changes from 1 commit
dd028bf
a317407
443164c
faea784
191a6ec
f6276bc
4f629fa
bbf8ac1
4a54b4c
0995694
88b6684
29514ff
22b04ae
dcafcee
8747556
dfaa27b
c528dce
a5b28e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,25 +23,25 @@ public class OdpManager | |
{ | ||
private volatile OdpConfig _odpConfig; | ||
|
||
public OdpSegmentManager SegmentManager { get; } | ||
public IOdpSegmentManager SegmentManager { get; } | ||
|
||
public OdpEventManager EventManager { get; } | ||
public IOdpEventManager EventManager { get; } | ||
|
||
private ILogger _logger; | ||
|
||
public OdpManager(OdpConfig odpConfig, OdpSegmentManager segmentManager, | ||
OdpEventManager eventManager, ILogger logger = null | ||
public OdpManager(OdpConfig odpConfig, IOdpSegmentManager segmentManager, | ||
IOdpEventManager eventManager, ILogger logger = null | ||
) | ||
{ | ||
_odpConfig = odpConfig; | ||
SegmentManager = segmentManager; | ||
EventManager = eventManager; | ||
_logger = logger; | ||
_logger = logger ?? new NoOpLogger(); | ||
|
||
EventManager.Start(); | ||
} | ||
|
||
public bool UpdateSettings(string apiHost, string apiKey, List<string> segmentsToCheck) | ||
public bool UpdateSettings(string apiKey, string apiHost, List<string> segmentsToCheck) | ||
{ | ||
var newConfig = new OdpConfig(apiKey, apiHost, segmentsToCheck); | ||
if (_odpConfig.Equals(newConfig)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can't we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes Sir. I'll update once using the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried overriding the implicit operator, but it gets messy and unclear. I'd like to stay with the use of |
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason to add this method. This will raise exception if ApiKey is null. Secondly use equals method where needed in this class and implement IEqu... interface as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal of this is to control the equality determination of two ODP Configs.
Thanks for the callout on null exception. I'll change to use
IEqualityComparer
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh maybe
IEquatable<T>
might work instead of having a comparer. 🤔