smashwilson / slack-emojinator

Bulk upload emoji into Slack
MIT License
328 stars 52 forks source link

Follow pagination for >500 emoji #23

Closed d-lord closed 6 years ago

d-lord commented 6 years ago

Hi! A friend mentioned that the export script wasn't catching every emoji any more.

At some point, Slack started paginating on the export page for teams with huge emoji collections. This means the export process would only detect the 500 emoji on that first page (ordered alphabetically). It now looks to other pages and exports those as well, restoring full functionality.

Tested on a team with ~1300 emoji (3 pages).

(I've also commented/renamed/added type signatures to methods & ran PyCharm's built-in "optimize imports" to tidy that, hence the changes at the top of the file)

mitchmcdee commented 6 years ago

I'm successfully parsing all 1300 emojis now :) however, I still get this error:

Traceback (most recent call last):
  File "export.py", line 128, in <module>
    loop.run_until_complete(main())
  File "/usr/local/Cellar/python/3.6.5/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/base_events.py", line 468, in run_until_complete
    return future.result()
  File "export.py", line 118, in main
    http_get = concurrent_http_get(args.concurrent_requests, session)
  File "export.py", line 49, in concurrent_http_get
    semaphore = asyncio.Semaphore(max_concurrent)
  File "/usr/local/Cellar/python/3.6.5/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/locks.py", line 426, in __init__
    if value < 0:
TypeError: '<' not supported between instances of 'str' and 'int'
mitchmcdee commented 6 years ago

fyi I fixed this error by wrapping max_concurrent with int() lol, would have thought typing would have caught that?

mitchmcdee commented 6 years ago

Why isn't upload async? :o

mitchmcdee commented 6 years ago

apart from that looks like everything worked! :) thank you!

d-lord commented 6 years ago

I guess you're using the CONCURRENT_REQUESTS environment variable? Looks like I didn't test that at all 😩 It comes in as a string. I've reproduced & fixed that, please try again.

mitchmcdee commented 6 years ago

hang on I'm doing bulk upload bahahah, but that was my issue so I'm guessing its fixed! still confused why typing didn't crap its pants? was it meant to?

d-lord commented 6 years ago

My understanding is that typing is for static analysis, and is pretty toothless at runtime. Guessing the static analysis doesn't pick up on "argparse namespaced attribute may not be an int".

As to why upload isn't async - I haven't bothered! When I added async to export, my home internet could do heaps more than one simultaneous download, but had horrendous upload. This is @smashwilson's project and he already had a perfectly good upload script for my needs 😇

d-lord commented 6 years ago

Hi @smashwilson, any objection to me merging this on the main branch?

smashwilson commented 6 years ago

@d-lord None at all, merge at will 🚀

d-lord commented 6 years ago

Cheers! ⚡️