Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Districtm Dmx: new adapter #1209
Districtm Dmx: new adapter #1209
Changes from 15 commits
0d36837
a098d7c
255d384
c78cdcc
436f3a5
49bcf69
419bfc6
f04450f
b3e6fbc
0f4e542
08f108d
afb9768
bd77380
8e27b0b
63b61d2
572cc96
8bc4bd2
4e41153
a00c90c
9e4693d
b53697a
b6530cb
311e106
aa1dd6e
465ab83
f906587
31d3f0b
5f0339e
3dcb2dc
30c595d
409caec
f655384
c3f6364
c851a32
d7bb136
9e6fe59
711e779
cbbd517
2b82610
e649de1
4bf0745
0284310
8ed9ec3
b6ed6da
2f3c122
76372d0
a4ac24f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 don't see
publisherId
in this struct being used for anything? It should not belong there anyway, since there will be only one instance of theDmxAdapter
across all requests, and I imagine thatpublisherId
is more dynamic than that.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.
@stevealliance This comment needs to be addressed.
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.
Please verify there is at least 1 Format in the collection before indexing it via
inst.Banner.Format[0]
.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.
Consider simplifying the case here with a single if clause.
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.
We've updated the template for this test. Did you get this old style from a doc we forgot to update?
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.
Please consider using
ExtraAdapterInfo
for bidder specific configs. We added that recently to avoid many one-off config entries.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, best to avoid one off additions to adapter configs.
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.
Curious. Do you work for DistrictM?
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'm asking because you're using a personal email address. It's preferable to be a work email address or a work support mailing list.