-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature/emoticon downloader #48
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.
Great that you ported it as a command. I just have a few comments.
self.logger.debug('Writing to disk {}({}) -> {}'.format(url, content_length, self.local_save_path)) | ||
with open(local_save_path, 'wb') as f: | ||
for chunk in res.iter_content(1024): | ||
f.write(chunk) |
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.
We should probably implement a Downloader
class of some kind, or add to slacker.utility
, at some point to encapsulate this function's functionality so it can be reused for other stuff.
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.
Agreed. I think we need to extend the project to have useful utility functions
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.
|
||
for emoji in emojis: | ||
emoji_url = emojis[emoji] | ||
if emoji_url[:5] == 'alias': |
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.
You can use emoji_url.startswith('alias')
for readability and to be sure 5 doesn't stay unchanged if the word "alias" does, for instance.
continue | ||
|
||
saved_file_path = self.__download_emoji(emoji, emojis[emoji]) | ||
save_data[emoji] = saved_file_path |
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.
A nice-to-have for later would be a concurrent download of all emojis to speed things up.
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.
I agree. I will add a task for this
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.
|
||
res = requests.get(url, stream=True) | ||
if res.status_code != 200: | ||
self.logger.warning('Unable to download {}: {}'.format(emoji_name, url)) |
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.
We probably need to support HTTP redirects (3xx).
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.
Ah, nevermind. I just found that follow_redirects = True
per default: http://docs.python-requests.org/en/master/user/quickstart/#redirection-and-history
|
||
def __write_save_file(self, save_data): | ||
with open(self.save_data_file, 'w') as f: | ||
f.write(json.dumps(save_data, indent=2)) |
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.
Any reason you don't do json.dump(save_data, f, indent = 2)
?
self.logger.info('Contacting Slack API for emojis') | ||
|
||
slack_api = SlackAPI() | ||
emojis = slack_api.post('emoji.list')['emoji'] |
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.
I like to check the 'ok'
in responses to see if things went as they should, and to print the 'error'
if it did not.
Perhaps SlackAPI.post
should return an object so we can do something like this:
slack_api = SlackAPI()
resp = slack_api.post('emoji.list')
if not resp.ok():
self.logger.error(resp.error())
emojis = resp.json['emoji']
It would be great to make it easy to check for success. Another way would be for SlackAPI
to simply throw an exception if not "ok" and an error is defined?
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.
We can create a task for extending the SlackAPI
class to handle network errors. Another case of failure would be when the API returns malformed JSON in rare cases, that would cause an exception to be thrown and we aren't catching it.
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.
slack_api = SlackAPI() | ||
emojis = slack_api.post('emoji.list')['emoji'] | ||
|
||
if args.quiet == False: |
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.
You could also write if not args.quiet:
if args.quiet == False: | ||
self.print_emojis(emojis) | ||
|
||
if args.path is not None: |
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.
if args.path:
is a little shorter.
|
||
self.logger.info('Emoji image files saved to {}'.format(self.local_save_path)) | ||
cwd_save_data = os.path.join(os.getcwd(), self.save_data_file) | ||
self.logger.info('Emoji save data file saved to {}'.format(cwd_save_data)) |
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.
Shouldn't the JSON file be saved to where --path
points to if defined?
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.
I thought about it, but it is nice that it is saved relative to where you run slacker. The save data file also includes the path to where the emoji was downloaded. Perhaps later we can create a command to do something with the emoji once they've been downloaded, like import to another workspace or something? Unfortunately Slack doesn't provide methods for creating custom emojis yet, so not sure what to do once downloaded :/
{
"bowtie": "/tmp/bowtie.png",
"squirrel": "/tmp/squirrel.png",
"glitch_crab": "/tmp/glitch_crab.png",
"piggy": "/tmp/piggy.png",
"cubimal_chick": "/tmp/cubimal_chick.png",
"dusty_stick": "/tmp/dusty_stick.png",
"slack": "/tmp/slack.png",
"pride": "/tmp/pride.png",
"thumbsup_all": "/tmp/thumbsup_all.gif",
"slack_call": "/tmp/slack_call.png",
"shipit": "alias:squirrel",
"white_square": "alias:white_large_square",
"black_square": "alias:black_large_square",
"simple_smile": "/tmp/simple_smile.png",
"metal": "/tmp/metal.png"
}
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.
That's a good point. Let's keep it as-is for now. It's a little annoying we can't import via their API, yes. :/ But I still think it's useful because it is then easier to add manually to another workspace.
387122f
to
c5f46d4
Compare
@netromdk updated |
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.
Very good. I'll merge.
Converts emoticon download script to an actual command that can be ran in REPL.
Command usage text: