Opt out phones in EA use case and sample script #670
Conversation
mkwoods927
added
the
sample script
|
@shaunagm I'm worried about line 121 in the sample script because I've been told it's not best practice to do a literal eval like that, but I'm also not sure how else to accomplish what that line does! If you have suggestions I'd love to hear. Thanks! |
There was a problem hiding this comment.
This looks great! I do have a bunch of relatively minor suggestions. The main issue I had reading the sample script was in understanding what schema.tables were. I also made a suggestion about restructuring the attempt_optout function that may or may not be necessary—I'm not sure if my instinct that using multiple return statements is more readable, is correct.
| ) | ||
| raise Exception(f"Connection Error {r}") | ||
|
|
||
| return r |
There was a problem hiding this comment.
I could be misunderstanding the construction of the function, but it feels like you can use return directly within the try statement and the two except loops, rather than storing the response as a variable in each segment then returning at the end of the function. I think that's more clear?
|
Ok @shaunagm I think this is all set! It looks like the CircleCI tests failed on an error in an unrelated test, but let me know if there's something I should change here! |
|
I suspect that if you rebase, that unrelated error will go away. Have you ever rebased something before? According to the docs there should be a rebase branch button somewhere but I can't find it, so we may need to do this manually. |
|
It looks like that option might not be enabled for this repo!
|
|
@shaunagm I am seeing a message saying "this branch has no conflicts with the base branch" and it looks like squash and merge is clickable though |
|
I was able to rebase the PR for you @mkwoods927 but it looks like there are still some linting issues: Can you fix these? I forget whether we went over linting together, let me know if you need any help. (Also apologies that we've yet to implement automatic linting with Black.) |
|
Yes I can fix these next week! Thanks for doing the rebase!Sent from my iPhoneOn Dec 8, 2022, at 1:45 PM, Shauna ***@***.***> wrote:
I was able to rebase the PR for you @mkwoods927 but it looks like there are still some linting issues:
useful_resources/sample_code/opt_outs_everyaction.py:18:14: F821 undefined name 'json'
useful_resources/sample_code/opt_outs_everyaction.py:51:101: E501 line too long (104 > 100 characters)
useful_resources/sample_code/opt_outs_everyaction.py:66:20: F821 undefined name 'ea'
useful_resources/sample_code/opt_outs_everyaction.py:67:1: W293 blank line contains whitespace
useful_resources/sample_code/opt_outs_everyaction.py:76:1: W293 blank line contains whitespace
useful_resources/sample_code/opt_outs_everyaction.py:90:1: W293 blank line contains whitespace
useful_resources/sample_code/opt_outs_everyaction.py:106:101: E501 line too long (113 > 100 characters)
useful_resources/sample_code/opt_outs_everyaction.py:116:1: W293 blank line contains whitespace
useful_resources/sample_code/opt_outs_everyaction.py:128:1: W293 blank line contains whitespace
useful_resources/sample_code/opt_outs_everyaction.py:146:1: W293 blank line contains whitespace
useful_resources/sample_code/opt_outs_everyaction.py:150:1: W293 blank line contains whitespace
useful_resources/sample_code/opt_outs_everyaction.py:164:101: E501 line too long (102 > 100 characters)
Can you fix these? I forget whether we went over linting together, let me know if you need any help.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
@shaunagm I think I've got all the linting issues fixed! For some reason the docs-build part of the test is failing, but I don't understand the error. Is this something I need to resolve? Error: [Errno 2] No such file or directory: '/home/circleci/project/venv/bin/python3' |
|
Hmm, I suspect the issue is something like this one and someone from TMC needs up update the CircleCI configuration (maybe by updating/"bursting" the Cache keys? whatever that means). Tagging @neverett to get this on their radar. Definitely not anything for you to worry about @mkwoods927. |
This PR adds a new use case and sample script for opting out phone numbers in EveryAction.