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

Update fundraise-up-to-basket-newsletter-signup.py #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mtdenton
Copy link
Contributor

Using this commit just for review purposes

Using this commit just for review purposes
Copy link

@jhonatan-lopes jhonatan-lopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mtdenton, I don't see anything egregious here 😆 it's looking good.

Minor nitpick, url_parts instead of urlParts would be more pythonic (snake_case).

If you're looking for improvement suggestions, though, I have some tips:

To get the language from the url, instead of splitting it directly, you could first parse it with the urlparse module. So instead of

    urlParts = input_data['source_url'].split("/")
    lang = urlParts[3]

you could do:

from urllib.parse import urlparse

url_parts = urlparse(input_data['source_url'])
lang = url_parts.path.split("/")[1]  # .path gives us '/en/...', so split it and get the second result since it starts with a /

This would ensure that the url is "parseable" and gives us better checks that we start with the path to look for the language, instead of trying to get the language from the whole url.

Another thing I was going to ask is to clarify how this script is going to be called. How would you pass input_data to the script? Are you going to wrap this script into a function? Is this TBD work?

@mtdenton
Copy link
Contributor Author

Hey @jhonatan-lopes! This is perfect, thank you for the feedback!

Definitely updating to url_parts. You can take the dev out of JavaScript, but you can't take the JavaScript out of the dev.

Zapier is strange. Like a lot of other "send us your snippet to execute" services like this, this code gets injected into some sandbox that already has a bunch of things set up. For instance, input_data and output is already initialized and/or assigned by the time this code executes and Zapier instructs you to utilize these to interact with other steps in the Zap.

Which also means that you can't import packages aside from the ones that they already allow. I am not sure if urllib is one of those packages already present in the sandbox but I'm taking a look.

All of these questions are going to make great docs for this for anyone who runs across this weirdness!

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

Successfully merging this pull request may close these issues.

2 participants