Skip to content
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

Added support for Twitter Ads API #403

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

nouvak
Copy link

@nouvak nouvak commented Nov 2, 2015

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.

@michaelhelmick
Copy link
Collaborator

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 :)

@nouvak
Copy link
Author

nouvak commented Nov 19, 2015

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
Copy link
Collaborator

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

@nouvak
Copy link
Author

nouvak commented Dec 2, 2015

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
Copy link

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

@karambir
Copy link
Contributor

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

@michaelhelmick
Copy link
Collaborator

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()

@nouvak
Copy link
Author

nouvak commented May 3, 2016

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
Copy link
Collaborator

@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
Copy link
Author

nouvak commented May 9, 2016

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants