adi-DATEX-472-publish-data-apis-sdk#1
Conversation
|
|
||
|
|
||
| with Ttddata( | ||
| server_url="https://api.example.com", |
There was a problem hiding this comment.
if appropriate, i recommend setting up a default server (if there is a default server—I don't really know what the data API does). See workflows SDK: https://github.com/thetradedesk/ttd-workflows-python/blob/main/README.md#server-selection
There was a problem hiding this comment.
Thanks, I'll look to add a more comprehensive readme. Default Server is appropriate here, I'll be looking into what fits appropriately here.
There was a problem hiding this comment.
The default here points to an example endpoint though. The default should be something that actually works.
There was a problem hiding this comment.
The USAGE.md file is generated by speakeasy. In the next iteration, I'll look into editing the speakeasy configs to ensure any changes to this file is not overwritten by speakeasy. That will help ensure once we set a default server, it isnt overwritten.
There was a problem hiding this comment.
I have added a default URL in the file.
29a840d to
5102a1b
Compare
5102a1b to
507a37c
Compare
| from ttd_data import TTDData | ||
|
|
||
| sdk = TTDData( | ||
| # SDK arguments |
There was a problem hiding this comment.
I think you still need to set up auth? I'd think this would take an auth token
| devContainers: | ||
| enabled: true | ||
| schemaPath: .speakeasy/out.openapi.yaml | ||
| sdkClassName: TTDData |
There was a problem hiding this comment.
I am not too sold on TTDData, perhaps DataClient might be a better fit since TTD comes up in the package name already so it's clear that the client is from TTD.
There was a problem hiding this comment.
Same here, open to renaming it to TTDDataClient, for reason being that once its imported as
from ttd_data import DataClient, its not clear reading DataClient.DoSomething() to understand where its coming from.
Having TTD part of the client name makes it clear when reading code.
| asyncMode: both | ||
| authors: | ||
| - Speakeasy | ||
| baseErrorName: TTDDataError |
There was a problem hiding this comment.
In spirit of the above how about DataError and ApiError?
There was a problem hiding this comment.
I feel DataError and ApiError are too generic. TTDDataError is clear by name, where the error came from. I am in favor of editing ApiError to be TTDApiError to match. But generic names like DataError, ApiError may conflict with other libraries which may have generic error names.
| | Variable | Description | | ||
| |---|---| | ||
| | `TTD_AUTH_TOKEN` | **(Required)** Your TTD authentication token | | ||
| | `TTD_DATA_SERVER_URL` | API server URL (defaults to `https://api.example.com`) | |
There was a problem hiding this comment.
does this needs to be regenerated? Does not seem to be accurate anymore
| source .venv/bin/activate | ||
| pip install -e . | ||
| export TTD_AUTH_TOKEN="your-ttd-auth-token-here" | ||
| export TTD_DATA_SERVER_URL="https://usw-data.adsrvr.org" |
There was a problem hiding this comment.
Why did we pick usw-data.adsrvr.org as a default?
There was a problem hiding this comment.
https://partner.thetradedesk.com/v3/portal/data/doc/DataEnvironments#first-pd-servers
Referred to this while setting it up, all first-party dataservers are region specific. I can change it to https://bulk-data.adsrvr.org that seems like the only generic name but is confusing since our documentation does not state it as first party dataserver.
| | `ttd_auth` | *Optional[str]* | :heavy_minus_sign: | Data API token for authentication. If not provided, TtdSignature is required. | | ||
| | `ttd_signature` | *Optional[str]* | :heavy_minus_sign: | Legacy signature-based authentication. Required if TTD-Auth is not provided. | | ||
| | `data_provider_id` | *OptionalNullable[str]* | :heavy_minus_sign: | N/A | | ||
| | `items` | List[[models.AdvertiserDataItem](../../models/advertiserdataitem.md)] | :heavy_minus_sign: | N/A | |
There was a problem hiding this comment.
Tangential but I feel like I should look into why this is coming as optional from swagger
| import pydantic | ||
| from ttd_data.errors import TTDDataError | ||
| from ttd_data.models import ( | ||
| advertiserdataserverresponseline as models_advertiserdataserverresponseline, |
There was a problem hiding this comment.
this is a very ugly name, is there some policy we can tweak so that this gets generated with some _ in between or smth.
There was a problem hiding this comment.
This has to do with how its named in swagger. We can override it and change some setting in speakeasy to ensure that it is not overwritten everytime speakeasy generates a new version.
There was a problem hiding this comment.
yep, i agree this would be a good improvement, we do this in workflows if you need a model or the speakeasy docs should have guidance
|
|
||
|
|
||
| with TTDData( | ||
| server_url="https://api.example.com", |
There was a problem hiding this comment.
still see a lot of these and no default alias'ed URL 🤔
There was a problem hiding this comment.
discussed this in the slack thread, there should be a follow-up to set a default / alias
feec7aa to
3515943
Compare
What does this MR do?
Local Tests:
