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 time.Now() as the first parameter of NewRates() #1953

Merged
merged 2 commits into from
Aug 18, 2021

Conversation

guscarreon
Copy link
Contributor

The DataAsOf time.Time field of the Rates struct found in currency/rates.go has no use in practice because (rc *RateConverter) update() method sets the RateConverter's lastUpdated field to time.Now() in line 82 shown below and ignores the rates.DataAsOf field that the fetch() function unmarshals in line 79. This pull request removes the DataAsOf field entirely which is useful in order to simplify the code specially in the numerous NewRates() calls throughout the codebase.

 77 // Update updates the internal currencies rates from remote sources
 78 func (rc *RateConverter) update() error {
 79     rates, err := rc.fetch()
 80     if err == nil {
 81         rc.rates.Store(rates)
 82         rc.lastUpdated.Store(rc.time.Now())
 83     } else {
 84         if rc.checkStaleRates() {
 85             rc.clearRates()
 86             glog.Errorf("Error updating conversion rates, falling back to constant rates: %v", err)
 87         } else {
 88             glog.Errorf("Error updating conversion rates: %v", err)
 89         }
 90     }
 91
 92     return err
 93 }
 94
 95 func (rc *RateConverter) Run() error {
 96     return rc.update()
 97 }
currency/rate_converter.go

Copy link
Collaborator

@bsardo bsardo left a comment

Choose a reason for hiding this comment

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

Looks good; just a nitpick.

@@ -143,7 +143,6 @@ func TestReadWriteRates(t *testing.T) {
} else {
rates := currencyConverter.Rates().(*Rates)
assert.Equal(t, tt.wantConversions, (*rates).Conversions, tt.description)
assert.Equal(t, tt.wantDataAsOf, (*rates).DataAsOf, tt.description)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should also be able to delete wantDataAsOf in the tests struct in this test TestReadWriteRates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're totally right. Removed

Copy link
Collaborator

@bsardo bsardo left a comment

Choose a reason for hiding this comment

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

LGTM

@bsardo bsardo merged commit 744bf42 into prebid:master Aug 18, 2021
wwwyyy pushed a commit to wwwyyy/prebid-server that referenced this pull request Aug 20, 2021
… prebid-master

* 'master' of https://github.com/prebid/prebid-server:
  Update Viper (prebid#1969)
  /cookie_sync Endpoint Rewrite (prebid#1879)
  New Adapter: IQZone (prebid#1964)
  Remove old race conidtion tests (prebid#1958)
  Remove time.Now() as the first parameter of NewRates() (prebid#1953)

# Conflicts:
#	config/config.go
#	usersync/usersyncers/syncer_test.go
jizeyopera pushed a commit to operaads/prebid-server that referenced this pull request Oct 13, 2021
shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022
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.

3 participants