integrations/trello: Export zulip_trello as an installed script.

zulip / python-zulip-api

Python library for the Zulip API.

https://zulip.com/api/

Apache License 2.0

361 stars 362 forks source link

integrations/trello: Export zulip_trello as an installed script. #478

Closed eeshangarg closed 6 years ago

eeshangarg commented 6 years ago

@rishig: I would really appreciate it if you could please review this one! I'll also open up a PR that updates our Trello webhook docs in the main repo in a bit! Thanks! :)

@timabbott: FYI :)

eeshangarg commented 6 years ago

@rishig: Just updated this in response to your feedback. As for your questions, here are my thoughts:

I don't think we need to log anything here. The only information worth logging is the ID of the webhook created, which can just be printed to the screen. We shouldn't really rely on logging to an external file because where these files end up is platform-dependent, and users haven't really been a huge fan of having to find vague files on their system in order to set up a webhook.
Yeah, no the script would break in that instance. I can make it so that doesn't happen if you'd like me to, but multiple people have already complained about the old instructions, so it seems like too much work that might not be worth it! Let me know what you think! :)

Thanks! :)

eeshangarg commented 6 years ago

@rishig: Just updated this after testing the script thoroughly with both Python 2 and 3. Also, I'm not sure what we should do about the requests dependency? I'm guessing it is not a part of the standard library so we should probably replace it with urllib?

rishig commented 6 years ago

yeah, replacing with urllib seems reasonable, if it's not too much work. Otherwise lgtm!

timabbott commented 6 years ago

I think replacing it with urllib isn't going to work, because that has different APIs for Python 2 vs. 3, and then you're going to need six to handle that. Let's just add pip install requests to the docs, perhaps?

(I merged the doc changes; so we should get this in before those changes hit prod in a few days)

eeshangarg commented 6 years ago

@rishig, @timabbott: Just updated this with a commit! The script should now print an error message if requests isn't installed. :)

eeshangarg commented 6 years ago

@rishig: Done! Thanks for the review! :)

eeshangarg commented 6 years ago

@timabbott: Since the doc changes in the main repo have already been merged, I feel like I should merge this and make a PyPI release before the doc changes in the main repo hit production. :) Thanks!