Add Script to Update emojis from Slackmoji.com #123
Conversation
|
Well, apparently this is a WIP, since it is failing on 220 of them since they are defined elsewhere or are too large, etc. |
|
The too-large emojis should be reported back upstream to Slackmoji, as they won't be able to import them either! |
|
You might want to rename some of the slackmojis to include the pack name. The conflict between the NFL Giants and MLB Giants immediately comes to mind. |
|
i have some more local edits that i haven't pushed yet, but am also looking for a place to send feedback to the slackmojis.com maintainer. I started with a tweet though - https://twitter.com/jondrice/status/889925917602172928 |
|
Retweeted you. LMK where I can help and when this is ready 2 merge! |
|
Will do! I actually bothered to set up a local test environment too, so I don't have to rely on Travis to tell me I goofed. Whenever it is, the next commit that passes Travis should be good to go, but I'll post here too. |
|
Yay, it passed CI! I personally think it is ready to merge, but would like you to take a look first - at least at the subfolder in the |
|
I slept on it, and decided to make it a flat structure - so I took care of that this morning. I also addressed sports as special cases per @gtback's suggestion, so all teams are e.g. :nfl-giants: instead of just :giants:. This will make them easier to search for in Slack as well. Everything else that is duplicated just has an incrementing number at the end. I also reported the too-large emojis from slackmojis.com per @hugovk's suggestion and they have been removed/resized by the maintainer. I'd say this is for real ready to merge! |
|
I haven't tested this myself but if the tests pass, I'm for it. The code looks well organized and like it should be doing what you claim! @hugovk any thoughts before I merge? I know you were looking at this too. |
There was a problem hiding this comment.
Looking good! Just a few comments.
Also a few not-so-important style things:
update_slackmojis.py:11:80: E501 line too long (84 > 79 characters)
update_slackmojis.py:12:1: E302 expected 2 blank lines, found 1
update_slackmojis.py:17:1: E302 expected 2 blank lines, found 1
update_slackmojis.py:48:80: E501 line too long (83 > 79 characters)
update_slackmojis.py:50:5: E265 block comment should start with '# '
update_slackmojis.py:75:27: E201 whitespace after '{'
update_slackmojis.py:75:40: E202 whitespace before '}'
update_slackmojis.py:90:80: E501 line too long (83 > 79 characters)
update_slackmojis.py:110:80: E501 line too long (82 > 79 characters)
update_slackmojis.py:139:5: E265 block comment should start with '# '
README.md
Outdated
| - [Shiba Stickers](https://raw.githubusercontent.com/lambtron/emojipacks/master/packs/shiba.yaml) (from Messenger) | ||
| - [gamedevmoji](https://raw.githubusercontent.com/niksudan/gamedevmoji/master/gamedevicons.yaml) | ||
| - [AWS simple icons](https://raw.githubusercontent.com/Surgo/aws_emojipacks/master/noprefix-emojipacks.yml) | ||
|
|
||
| ### Emoji packs from [slackmojis.com](http://www.slackmojis.com) | ||
|
|
||
| - [Uncategorized](https://raw.githubusercontent.com/lambtron/emojipacks/master/packs/slackmojis-.yaml) |
There was a problem hiding this comment.
This is really slackmojis-uncategorized.yaml
| import io | ||
| import json | ||
| import os | ||
| from pprint import pprint |
There was a problem hiding this comment.
'pprint.pprint' imported but unused
| import json | ||
| import os | ||
| from pprint import pprint | ||
| import sys |
|
|
||
| def remove_file(f): | ||
| try: | ||
| print 'removing file', f |
There was a problem hiding this comment.
Not essential, but add from __future__ import print_function at the top and we can use the print() function in Python 2 as well as Python 3. It's good to be ready for both.
| yaml.dump(data, f, Dumper=MyDumper, default_flow_style=False) | ||
|
|
||
|
|
||
| def create_dirs(dir): |
There was a problem hiding this comment.
There's some duplicate code shared in this file and image-checker.py. Should we put common things in a utlis.py, and maybe put all the scripts in the same directory?
There was a problem hiding this comment.
Ummmm, I'll leave that up to someone else - or future me? seems like it could be a different pull request/issue.
|
@hugovk thanks so much for reviewing! |
|
yes, thanks for the code review. I'll tackle some of these later. The fixes should be pretty straightforward. |
|
@hugovk , I'm down to merge if you are. |
|
Make it so! Thanks! |
|
|
|
I know this was already merged, but would it be better for the scrabble pack to use |
|
I was thinking the same thing with facebook ones to start with the :fb- prefix. Maybe start another issue to map slackmoji categories to possible prefixes? |
* 'master' of ssh://github.com/lambtron/emojipacks: (30 commits) Added export json conversion script to easily transfer emojis between slacks (lambtron#141) introducing pumpkinparrot and angelparrot (lambtron#140) CI: Only print summary if errors (lambtron#138) Add pack with German states' (Bundesländer) coats of arms (lambtron#137) Fix pokemon to fix CI (lambtron#139) five new parrots (lambtron#136) add luckyparrot and rotatingparrot (lambtron#133) Delete hipchat.yaml (lambtron#135) Update README.md with 2FA (lambtron#132) Added in ryangoslingparrot and tacoparrot (lambtron#131) Add Slackmoji prefixes (lambtron#127) added 11 new parrots (lambtron#128) Add Script to Update emojis from Slackmoji.com (lambtron#123) Updated file extension to fit with convention (lambtron#124) Add AWS Simple icons (lambtron#120) Update Finland pack with new emoji (lambtron#121) Revert "created an all emoji file (lambtron#118)" (lambtron#119) created an all emoji file (lambtron#118) Added prefix to occupy emoji. [lambtron#116] (lambtron#116) Adds Political Pack of Emojis (lambtron#115) ...
This should resolve #122, which I just wrote up.