-
Notifications
You must be signed in to change notification settings - Fork 753
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
[Currency support] Adding a debug endpoint #822
[Currency support] Adding a debug endpoint #822
Conversation
This CL introduces a new admin endpoint exposing current currency rate converter internals such as: - Sync source URL - Internal rates - Update frequency - Last update It will help for the future rollout of currencies support. More details: prebid#280
05eb3f1
to
c29e908
Compare
) | ||
|
||
// currencyRatesInfo holds currency rates information. | ||
type currencyRatesInfo struct { |
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.
Took me a while to figure out why you have two info structs. Finally realized because they are private, but in different packages.
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.
Yes indeed, I introduced it in order not to have the endpoint package tied to the currency package implementation :)
It's also easier to test
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.
LGTM
|
||
infos := rateConverter.GetInfo() | ||
if infos == nil { | ||
return currencyRatesInfo |
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.
Does it make sense for Active to be true but nothing else in the struct to be defined in this case?
Hey !
I think so in the sense where there is a rate converter set rather than
nil. The converter might be constant but it’s active and not the default
nil behavior
What do you think ?
Cheers !
…On Tue 12 Mar 2019 at 22:09, josephveach ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In endpoints/currency_rates.go
<#822 (comment)>:
> +// newCurrencyRatesInfo creates a new CurrencyRatesInfo instance.
+func newCurrencyRatesInfo(rateConverter rateConverter) currencyRatesInfo {
+
+ currencyRatesInfo := currencyRatesInfo{
+ Active: false,
+ }
+
+ if rateConverter == nil {
+ return currencyRatesInfo
+ }
+
+ currencyRatesInfo.Active = true
+
+ infos := rateConverter.GetInfo()
+ if infos == nil {
+ return currencyRatesInfo
Does it make sense for Active to be true but nothing else in the struct to
be defined in this case?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#822 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABVEg4HtNwXy5JP7TEjxd6tMTsGamt5Sks5vWBd4gaJpZM4ba9KM>
.
|
This CL introduces a new admin endpoint exposing current currency rate converter internals such as: - Sync source URL - Internal rates - Update frequency - Last update It will help for the future rollout of currencies support. More details: prebid#280
This CL introduces a new admin endpoint exposing current currency rate converter internals such as: - Sync source URL - Internal rates - Update frequency - Last update It will help for the future rollout of currencies support. More details: prebid#280
This CL introduces a new admin endpoint exposing current currency rate converter internals such as: - Sync source URL - Internal rates - Update frequency - Last update It will help for the future rollout of currencies support. More details: prebid#280
This CL introduces a new admin endpoint exposing current currency rate
converter internals such as:
It will help for the future rollout of currencies support.
More details: #280