-
Notifications
You must be signed in to change notification settings - Fork 731
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
InMobi: mtype support #3921
InMobi: mtype support #3921
Conversation
Code coverage summaryNote:
inmobiRefer here for heat map coverage report
|
Code coverage summaryNote:
inmobiRefer here for heat map coverage report
|
adapters/inmobi/inmobi.go
Outdated
default: | ||
return openrtb_ext.BidTypeBanner |
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.
It appears you were defaulting to banner before adding mtype
support but now that you have it, is your server supposed to always set it? If so, you might want to do what most other adapters do which is to return an error if mtype
does not equal one of your supported formats.
default:
return "", fmt.Errorf("unrecognized bid type in response from inmobi for bid %s", bid.ImpID)
and then in the calling code:
mediaType, err := getMediaTypeForImp(sb.Bid[i].ImpID, internalRequest.Imp)
if err != nil {
return nil, []error{err}
}
Thoughts?
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.
Sounds good. I've made the recommended changes.
Code coverage summaryNote:
inmobiRefer here for heat map coverage report
|
adapters/inmobi/inmobi.go
Outdated
case openrtb2.MarkupAudio: | ||
return openrtb_ext.BidTypeAudio, 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.
You haven't declared audio support in your YAML file so it might be best to remove this and let this case result in an error.
Code coverage summaryNote:
inmobiRefer here for heat map coverage report
|
@przemkaczmarek , can you please re-check this PR when you have a chance? There were a few improvements made which dismissed your review. |
No description provided.