Code review comment for lp:~the-dod/sahana-eden/twitter-oauth

Revision history for this message
Fran Boon (flavour) wrote :

Hi,

This work looks great - you're really getting to grips with the framework :)

I'm not sure about the naming of these settings:
deployment_settings.oauth.consumer_key=""
deployment_settings.oauth.consumer_secret=""

Would we now have a different oauth consumer key/secret for other applications?
If so then we should have twitter in the name to differentiate, e.g.:
deployment_settings.twitter.oauth_consumer_key=""
deployment_settings.twitter.oauth_consumer_secret=""

& hence:
def get_twitter_oauth_consumer_key(self):
def get_twitter_oauth_consumer_secret(self):

I'm happy to make the code changes & all below, just want clarity on your understanding.

I'm not sure I like assuming that non-GETs are POSTs, we do support HTTP PUT & DELETE too.

I'd personally prefer to be told about a missing library before a missing configuration as this can be a more significant blocker.

Also a few PEP8 cleanups needed (spaces after commas)- just try to bear it in mind for new code.

Many thanks - help clarify my understanding of OAuth & then I can merge/cleanup :)

F

review: Needs Information

« Back to merge proposal