Added support for Twitter Ads API

nouvak commented

2015-11-02 01:31:04 -08:00

(Migrated from github.com)

Hello!!

I added a subset of the Twitter Ads API (https://dev.twitter.com/ads/overview) to Twython.
The most commonly used functions for managing campaigns and creatives, as well as one function for the retrieval of statistics were added.

I also implemented the integration tests which I used to make sure the API works properly.

If you think patch provides an added value to Twython, I would be happy if you included it to the main branch. I tried to follow the coding style as much as possible, however if there are some issues in the code, please let me know.

Note: There is some duplication of the code in api.py and api_ads.py: for the first part, I didn't want to mess with the "api.py" stuff, so I simply created a copy of the file and adapted it to the Twitter Ads API. With minimal rewrite, those files could be merged together. This could be done later.

Hello!! I added a subset of the Twitter Ads API (https://dev.twitter.com/ads/overview) to Twython. The most commonly used functions for managing campaigns and creatives, as well as one function for the retrieval of statistics were added. I also implemented the integration tests which I used to make sure the API works properly. If you think patch provides an added value to Twython, I would be happy if you included it to the main branch. I tried to follow the coding style as much as possible, however if there are some issues in the code, please let me know. Note: There is some duplication of the code in api.py and api_ads.py: for the first part, I didn't want to mess with the "api.py" stuff, so I simply created a copy of the file and adapted it to the Twitter Ads API. With minimal rewrite, those files could be merged together. This could be done later.

👍 1

michaelhelmick commented

2015-11-07 07:16:19 -08:00

(Migrated from github.com)

The code looks fine, but as you said, the ad stuff was split off into different files -- I'd rather just have the merge of the files done in this branch before merging upstream.

Is there any difference in TwythonAds and Twython or did you just copy the file?

You can just use the Twython object from api.py and delete the api_ads.py if so.

Also, you can move all the endpoint methods from endpoints_ads.py to endpoints.py.

Something like this:

class EndpointsMixin(object):
    # Timelines
    def get_mentions_timeline(self, **params):
        ...

    ...

    # Ads
    def get_accounts(self, **params):
        response = self.get('accounts', params=params)
        return response['data']

Changing these, you might have to change some of the tests (even though they'll be skipped)

Last note, normally, I would question why the tests were written in the old form (against the real api, we moved to using responses and using fake API requests), but I'll accept them since they will be useful when transitioning the tests at a later date.

Again, everything looks pretty solid. Just asking to merge, delete those files and rearrange code from the files mentioned :)

The code looks fine, but as you said, the ad stuff was split off into different files -- I'd rather just have the merge of the files done in this branch before merging upstream. Is there any difference in `TwythonAds` and `Twython` or did you just copy the file? You can just use the `Twython` object from `api.py` and delete the `api_ads.py` if so. Also, you can move all the endpoint methods from `endpoints_ads.py` to `endpoints.py`. Something like this: ``` python class EndpointsMixin(object): # Timelines def get_mentions_timeline(self, **params): ... ... # Ads def get_accounts(self, **params): response = self.get('accounts', params=params) return response['data'] ``` Changing these, you might have to change some of the tests (even though they'll be skipped) Last note, normally, I would question why the tests were written in the old form (against the real api, we moved to using `responses` and using fake API requests), but I'll accept them since they will be useful when transitioning the tests at a later date. Again, everything looks pretty solid. Just asking to merge, delete those files and rearrange code from the files mentioned :)

nouvak commented

2015-11-19 03:28:26 -08:00

(Migrated from github.com)

Hi Michael!

Sorry for a quite late response, I had some other stuff to do and didn't have time to look into this.
As suggested, I merged the "api_ads.py" file into the "api.py" file. That file wasn't just a copy-paste of the "api.py" file, but contained some changes that had to be done in order to be able to communicate with Twitter Ads API. Could you please revise the the changes I did to "api.py" and let me know if they are ok to you?

Also, I decided to keep the "endpoints_ads.py" and "endpoints.py" files separate in order to keep the functions of Twitter Ads API and Twitter API separate. If you still think I should merge them, please let me know and I'll do that. :)

Regarding the tests, I used the integration testing during the implementation of Twitter Ads API, and unfortunately didn't have time to add unit tests as well. Hope this isn't too big of a problem...

Hi Michael! Sorry for a quite late response, I had some other stuff to do and didn't have time to look into this. As suggested, I merged the "api_ads.py" file into the "api.py" file. That file wasn't just a copy-paste of the "api.py" file, but contained some changes that had to be done in order to be able to communicate with Twitter Ads API. Could you please revise the the changes I did to "api.py" and let me know if they are ok to you? Also, I decided to keep the "endpoints_ads.py" and "endpoints.py" files separate in order to keep the functions of Twitter Ads API and Twitter API separate. If you still think I should merge them, please let me know and I'll do that. :) Regarding the tests, I used the integration testing during the implementation of Twitter Ads API, and unfortunately didn't have time to add unit tests as well. Hope this isn't too big of a problem...

michaelhelmick commented

2015-12-01 09:29:04 -08:00

(Migrated from github.com)

Sorry, I've been super busy. Still wanting to get this merged!

Please combine the ads endpoints into the endpoints.py but as a separate class (so, still as a MixIn)

Also, I'm trying to think of a more intuitive way for someone to use the API

Instead of

t = Twython(api_type=Twython.API_TYPE_TWITTER_ADS)

Maybe extending Twython: https://gist.github.com/michaelhelmick/7cd75a4cfacf795c497d to two separate classes is best? You won't have to do that. If you can just merge the endpoints into one file. I'll merge this. I'll be making some enhancements.

I think we might have a TwythonCore which holds all core stuff, then Twython which exclusively works with the Twitter REST API and then TwythonAds that exclusively works with the Twitter Ads REST API

Sorry, I've been super busy. Still wanting to get this merged! - Please combine the ads endpoints into the `endpoints.py` but as a separate class (so, still as a MixIn) Also, I'm trying to think of a more intuitive way for someone to use the API Instead of ``` python t = Twython(api_type=Twython.API_TYPE_TWITTER_ADS) ``` Maybe extending Twython: https://gist.github.com/michaelhelmick/7cd75a4cfacf795c497d to two separate classes is best? You won't have to do that. If you can just merge the endpoints into one file. I'll merge this. I'll be making some enhancements. I think we might have a `TwythonCore` which holds all core stuff, then `Twython` which exclusively works with the Twitter REST API and then `TwythonAds` that exclusively works with the Twitter Ads REST API

nouvak commented

2015-12-02 01:39:50 -08:00

(Migrated from github.com)

Ok, I combined the Ads endpoints into the endpoints.py file.

As for the TwythonCore, Twython and TwythonAds idea, I think it definitely makes sense! :)
At first, I also had the functionality split to Twython and TwythonAds classes, but had to remove them
when merging the code. So having the common code in TwythonCore and API-specific functionality in Twython and TwythonAds classes would be perfect. :)

But if I understood you correctly you will do those things, right?

Ok, I combined the Ads endpoints into the endpoints.py file. As for the TwythonCore, Twython and TwythonAds idea, I think it definitely makes sense! :) At first, I also had the functionality split to Twython and TwythonAds classes, but had to remove them when merging the code. So having the common code in TwythonCore and API-specific functionality in Twython and TwythonAds classes would be perfect. :) But if I understood you correctly you will do those things, right?

johendry commented

2016-01-26 04:44:30 -08:00

(Migrated from github.com)

am i need do apply 3 leves ads api application first?

karambir commented

2016-02-17 04:30:58 -08:00

(Migrated from github.com)

Nice PR. 👍 I also like the idea of TwythonCore and TwythonAds. But we should merge this PR then do another one for that.

Nice PR. :+1: I also like the idea of TwythonCore and TwythonAds. But we should merge this PR then do another one for that.

michaelhelmick commented

2016-04-30 04:32:27 -07:00

(Migrated from github.com)

Hi @nouvak,

So, I'm starting to tackle this and I was wondering, do you find a use case where a use will need to authenticate with Twitter then use Twitter Ads?

Do you think it would make more sense to keep the auth stuff in Twython class and then have them do Ad stuff with TwythonAds?

i.e.

t = Twython(...)
# do authentication
final_tokens = t.get_final_tokens() # I think that's what it's called? haha

ads = TwythonAds(key, secret, final_tokens['oauth_token'], final_tokens['oauth_token_secret'])
ads.some_method()

Hi @nouvak, So, I'm starting to tackle this and I was wondering, do you find a use case where a use will need to authenticate with Twitter then use Twitter Ads? Do you think it would make more sense to keep the auth stuff in `Twython` class and then have them do Ad stuff with `TwythonAds`? i.e. ``` python t = Twython(...) # do authentication final_tokens = t.get_final_tokens() # I think that's what it's called? haha ads = TwythonAds(key, secret, final_tokens['oauth_token'], final_tokens['oauth_token_secret']) ads.some_method() ```

nouvak commented

2016-05-02 23:41:15 -07:00

(Migrated from github.com)

Hi Mike!

Hmm, I'm not sure if I understood your question completely.
Current, we don't have a separate TwythonAds class, because we agreed the Twitter Ads functionality should be added to the Twython class.

Were you planning on creating a separate TwythonAds class?

Hi Mike! Hmm, I'm not sure if I understood your question completely. Current, we don't have a separate TwythonAds class, because we agreed the Twitter Ads functionality should be added to the Twython class. Were you planning on creating a separate TwythonAds class?

michaelhelmick commented

2016-05-05 18:54:28 -07:00

(Migrated from github.com)

@nouvak in your previous comment, you said:

Ok, I combined the Ads endpoints into the endpoints.py file.

As for the TwythonCore, Twython and TwythonAds idea, I think it definitely makes sense! :)

In response to my gist.

I was planning on creating separate classes.

@nouvak in your previous comment, you said: > Ok, I combined the Ads endpoints into the endpoints.py file. > > As for the TwythonCore, Twython and TwythonAds idea, I think it definitely makes sense! :) In response to my gist. I was planning on creating separate classes.

nouvak commented

2016-05-09 00:52:08 -07:00

(Migrated from github.com)

Oh I see. I totally forgot about your intention to separate the the Twython and TwythonAd class.

With regards to your question about authentication functionality, I would implement it in a separate class that would then be used by both, Twython and TwythonAd class.

The design I would implement:

the Twython class for working with Twitter API
the TwythonAd class for working with Twitter Ads API
i would implemente the functionalities that are shared by both API's (e.g. authentication, http calls, ...) in separate modules that would be used by both classes.

This is just a suggestion: you are free to do the implementation the way it makes the most sense to you.

Oh I see. I totally forgot about your intention to separate the the Twython and TwythonAd class. With regards to your question about authentication functionality, I would implement it in a separate class that would then be used by both, Twython and TwythonAd class. The design I would implement: - the Twython class for working with Twitter API - the TwythonAd class for working with Twitter Ads API - i would implemente the functionalities that are shared by both API's (e.g. authentication, http calls, ...) in separate modules that would be used by both classes. This is just a suggestion: you are free to do the implementation the way it makes the most sense to you.

ryanmcgrath commented

2020-04-02 23:43:12 -07:00

(Migrated from github.com)

As this is old and out of sync... if anyone wants to tackle getting it up and running be my guest.

This pull request has changes conflicting with the target branch.

tests/config.py
tests/test_endpoints.py
twython/api.py
twython/endpoints.py

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin nouvak/master:nouvak/master

git checkout nouvak/master

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git checkout master

git merge --no-ff nouvak/master

git checkout nouvak/master

git rebase master

git checkout master

git merge --ff-only nouvak/master

git checkout nouvak/master

git rebase master

git checkout master

git merge --no-ff nouvak/master

git checkout master

git merge --squash nouvak/master

git checkout master

git merge --ff-only nouvak/master

git checkout master

git merge nouvak/master

git push origin master

Rows
Columns

Added support for Twitter Ads API #403

Checkout

Merge