search

szastupov / aiotg

Asynchronous Python library for building Telegram bots
MIT License
381 stars 44 forks source link

Flask-like reloader for development #41

Closed creatorrr closed 7 years ago

creatorrr commented 7 years ago

Hey @szastupov, Been using aiotg in a project and it's been a charm! A flask-like reloader could be really helpful during development. I have put together a shabby implementation here. Do you think it is something you'd want to include in the module? If so, I can polish this PR further for a merge.

szastupov commented 7 years ago

Hey @creatorrr ! The PR looks promising so I would love to include this feature, thanks!

creatorrr commented 7 years ago

Sweet. Let me polish things up a bit and then push again.

creatorrr commented 7 years ago

Done. Tested informally but I think a unit test for this would be too complicated. Although, I can work on it later if you think it is needed.

creatorrr commented 7 years ago

Fixed all the review request changes but Travis is still failing ~saying ModuleNotFoundError: No module named 'aionotify'.~

@szastupov, I have added the deps to setup.py, do I need to do something else for them to work?

UPDATE: Travis works now except throwing a weird The command "flake8" exited with 1 error.

szastupov commented 7 years ago

Oh, snap, I forgot that CI uses requirements-dev.text, sorry (can't wait for Pipfile to become stable so we don't have to deal with requirements files anymore).

Just add modules to requirements-dev.txt without versions.

szastupov commented 7 years ago

UPDATE: Travis works now except throwing a weird The command "flake8" exited with 1 error.

Well, there are style errors :) It's not like I'm pep8 nazi but some style enforcement is better than nothing ;)

creatorrr commented 7 years ago

True that. I can't see which errors exactly in the CI log, ~can you point them out or should I just run flake8 on my branch?~

UPDATE: nvm, found and fixed.

creatorrr commented 7 years ago

All passing now!

szastupov commented 7 years ago

Looks good, nice job 👍

I was about to merge it but found out that aionotify is Linux-only, which is a major bummer since my primary dev machine is Macbook :(

creatorrr commented 7 years ago

Oh crap! Let me look if there is an easy option to fix that.

creatorrr commented 7 years ago

I think asphalt-filewatcher could do the job. Has multiple backends for different platforms. But this is a significant rewrite, so I need some time (could also use some help ;) ).

szastupov commented 7 years ago

asphalt-filewatcher looks a little bit too young. Have you checked watchdog? That's what aiohttp-devtools is using.

Anyway, I'll see if I can help. We have nowhere to rush 👌

creatorrr commented 7 years ago

Rewrote with watchdog. Should work like a charm now.

szastupov commented 7 years ago

Whoah dude, that was fast 👍

szastupov commented 7 years ago

Oh, one more thing! I guess we need to add some form of support for exclude lists because it gets triggered too often but maybe I'll just do it later myself, no worries.

creatorrr commented 7 years ago

Oh, one more thing! I guess we need to add some form of support for exclude lists because it gets triggered too often but maybe I'll just do it later myself, no worries.

You are right, the watcher was getting triggered way more often than intended. I was thinking of a graceful solution to this problem but for now, I have added the following whitelist to the watcher:

["*.py", "*.txt", "*.aiml", "*.json", "*.cfg", "*.xml", "*.html"] (common filetypes I expect to be part of an aiotg project)

This should be good enough for most use cases but it's up to you how you'd want to make this more configurable. Let me know what you think

szastupov commented 7 years ago

Great job, thank you 🙏