-
Notifications
You must be signed in to change notification settings - Fork 693
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 Dummy Terminology From Core Code #2007
Conversation
// Set will always return nil since this is a dummy service | ||
func (s *accountService) Set(account *cache.Account) error { | ||
return nil | ||
} |
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.
I was considering renaming this concept, when I noticed all implements just return nil
and is never called.
t.Errorf("RandomizeList, expected a list of 1, found %d", len(adapters)) | ||
// test all bidders are still present, ignoring order. we are testing the algorithm doesn't loose | ||
// elements. we are not testing the random number generator itself. | ||
assert.ElementsMatch(t, test.bidders, biddersWorkingCopy) | ||
} |
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.
Updated the test code while I was here removing "dummy" references.
type DummyMetricsEngine struct{} | ||
// NilMetricsEngine implements the MetricsEngine interface where no metrics are actually captured. This is | ||
// used if no metric backend is configured and also for tests. | ||
type NilMetricsEngine 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.
I also pondered MockMetricsEngine, but Nil may be better.
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.
That object already exists. It implements Testify's Mock library for some tests. This object throws away / ignores all metric calls.
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.
Some adapters still use Dummy
. I guess we could either their authors to remove it or remove it ourselves in a future PR. I believe we could remove Dummy
from adapters/appnexus/appnexus_test.go
ourselves without much hassle though. Do you think we should do it as part of this PR? (should be quick)
I didn't bother with changing references in code for the legacy auction endpoint since it will be removed in the very near future. |
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.
LGMT
* DummyMetricsEngine -> NilMetricsEngine * Legacy AccountsService.Set() Is Completely Unused * Remove Dummy Reference From Benchmark Tests * Remove Dummy Reference From Test * Misc Test References
* DummyMetricsEngine -> NilMetricsEngine * Legacy AccountsService.Set() Is Completely Unused * Remove Dummy Reference From Benchmark Tests * Remove Dummy Reference From Test * Misc Test References
* DummyMetricsEngine -> NilMetricsEngine * Legacy AccountsService.Set() Is Completely Unused * Remove Dummy Reference From Benchmark Tests * Remove Dummy Reference From Test * Misc Test References
Replaces the term "Dummy" with the more appropriate and professional "Nil" or "Test" in PBS-Core code. Except for the legacy cache, as that is tied to a host configuration variable and will be removed in the near future anyway.