-
Notifications
You must be signed in to change notification settings - Fork 132
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
Re-named Phone2Action to Capitol Canary #687
Re-named Phone2Action to Capitol Canary #687
Conversation
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.
These code changes look good. The only hesitation I have is that this is technically a Capitol Canary class wrapping Phone2Action, which means that at the future moment when we remove Phone2Action, we'll have to do the work of moving over the methods, instead of just deleting it.
There's no particular reason to do that now vs later, except that it'll be easier for our future selves. Up to you @bzupnick if you want to go ahead and make that change now or leave it for later.
Also, it would be good to merge the docs changes at the same time. Probably the best approach is:
(1) rename the Phone2Action rst doc for Capitol Canary, and change references to Capitol Canary at the top.
(2) create a new Phon2Action doc with the same name as the old name (so old links won't break) that says something like, "Phone2Action has been renamed Capitol Canary. All methods should be the same." with a link to the Capitol Canary docs.
@shaunagm I think you're right that the right thing to do is switch the "which class wraps which" relationship! Should be easy enough, especially with the tests set up. Also, I'll work on the docs. I'll post a code update later! |
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.
Thanks for making all these changes! Enjoy your vacation. :)
test/test_p2a.py
Outdated
@@ -127,7 +127,7 @@ class TestP2A(unittest.TestCase): | |||
|
|||
def setUp(self): | |||
|
|||
self.p2a = Phone2Action(app_id='an_id', app_key='app_key') | |||
self.p2a = Phone2Action(app_id='an_id', app_key='app_key').capitol_canary |
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.
Ahhh just noticed this. Sorry for assuming we were done! 😭
@bzupnick, can you leave this as self.p2a = Phone2Action(app_id='an_id', app_key='app_key')
? We want people to be able to keep using the Phone2Action class for now, with just a warning, which means we need to test that the wrapper is actually working by calling the class's methods directly, rather than testing .capitol_canary
's methods.
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.
The reason I did that was since below we use things like self.p2a.client.uri
which doesn't exist in p2a anymore.
But now that I write this out, I wonder if that means that my solution is incomplete and I should also bring in not just the functions, but also the class variables 🤔
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.
Let's try overriding __getattr__
. __getattr__
is a "magic method" that is only called when the attribute is not found. If we override it to look for the same attribute on CapitolCanary, that should move over all the attributes.
Should look something like this (this is from memory, the syntax may be off and its definitely oversimplified):
class Phone2Action:
self.__init__()
self.capitolcanary = CapitolCanary
self.__getattr__(self, name):
try:
return getattr(self.capitolcanary, name)
except:
raise AttributeError(f"{type(self).__name__} object has no attribute {name}")
The __getattr__
method is the only part you'd add.
Hi @bzupnick, want to try and pick this up again? Happy to pair on this last piece (and sorry again that I didn't notice it until after our last pairing session). |
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
This is a PR for issue #669.
I couldn't find API docs anywhere so this PR assumes all of the URLs and environment variables maintain the
phone2action
branding.