-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Nodals AI RTD Module : initial release #12649
Nodals AI RTD Module : initial release #12649
Conversation
Tread carefully! This PR adds 31 linter errors and 1 linter warning (possibly disabled through directives):
|
Tread carefully! This PR adds 1 linter error (possibly disabled through directives):
|
Tread carefully! This PR adds 1 linter error (possibly disabled through directives):
|
Tread carefully! This PR adds 1 linter error (possibly disabled through directives):
|
Tread carefully! This PR adds 1 linter error (possibly disabled through directives):
|
Tread carefully! This PR adds 1 linter error (possibly disabled through directives):
|
Hey @Fawke, Any chance this can get reviewed before the next release? |
Tread carefully! This PR adds 1 linter error (possibly disabled through directives):
|
Just an FYI: I have not edited |
Hey @ChrisHuie / @Fawke - I hope you're both well. Sorry to chase, all I'm really after is a rough ETA on when the review could begin? I do expect a little back and forth on this one, so I'm just trying to manage other folks' expectations. |
Tread carefully! This PR adds 32 linter errors (possibly disabled through directives):
|
Tread carefully! This PR adds 1 linter error (possibly disabled through directives):
|
Tread carefully! This PR adds 1 linter error (possibly disabled through directives):
|
Tread carefully! This PR adds 1 linter error (possibly disabled through directives):
|
Tread carefully! This PR adds 1 linter error (possibly disabled through directives):
|
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
All looks good from my end. Just want to confirm with @ChrisHuie that since this module is making a call to an external script, do they have to explicitly declare this somewhere in their documentation? |
Hey @ChrisHuie, Any chance you could help us out here? Is there anything else we need to provide to help this change make it into the next release? |
Hey @Fawke I've got a minor change that I'd like to make, but I do not want to risk this change preventing this module making it into the next release. If I make this change, would you be able to re-review within a day or two It's a one-liner. |
Actually, @Fawke - I have just gone over the rules again, and I see this: "A Real-Time Data module may load external code if it requires publisher registration and there’s a prominent disclosure on the module documentation." I will add this right now. Hopefully this will be enough to get this through. |
Tread carefully! This PR adds 1 linter error (possibly disabled through directives):
|
Tread carefully! This PR adds 1 linter error (possibly disabled through directives):
|
Tread carefully! This PR adds 1 linter error (possibly disabled through directives):
|
Tread carefully! This PR adds 1 linter error (possibly disabled through directives):
|
Hi @Fawke - sorry to chase again, just wondering if there is anything else you need from me? |
Documentation PR:
prebid/prebid.github.io#5812
Type of change
Bugfix
Feature
New bidder adapter
Updated bidder adapter
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Does this change affect user-facing APIs or examples documented on http://prebid.org?
Other
Description of change
RTD Module submission.
Other information
This RTD module will load external Javascript files which will determine the targeting data for the ad units. We are an early stage company and know that we will need to make changes quickly to how this component functions, so the external script files are how we will manage this for now. The unit tests stub the API exposed by the external script.
We are working with a few publishers who have requested that we get this module into the public repo prior to them being able to integrate.