Skip to content
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

Python 2 compatibility and redis fixes. #9

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

BillSchumacher
Copy link
Contributor

@BillSchumacher BillSchumacher commented Aug 15, 2018

Fixed - Issue where it would attempt to import redis in the store/redis.py overlapping naming...
Fixed - super() calls.
Fixed - An issue where py2 didn't like the class being the same type as renamed parent. Hence BeatXScheduler rename.
Fixed - Import urllib urlparse.
Fixed - Travis CI for Py2
Added - mock requirement for Py2
Added - Uppercase config detection, resolves #3.

@BillSchumacher
Copy link
Contributor Author

You have to squash them when you merge it in. I don't have merge permission.

@codecov-io
Copy link

codecov-io commented Aug 15, 2018

Codecov Report

Merging #9 into master will decrease coverage by 0.15%.
The diff coverage is 89.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #9      +/-   ##
==========================================
- Coverage   84.57%   84.41%   -0.16%     
==========================================
  Files          12       12              
  Lines         350      385      +35     
==========================================
+ Hits          296      325      +29     
- Misses         54       60       +6
Impacted Files Coverage Δ
setup.py 0% <ø> (ø) ⬆️
beatx/store/redis_store.py 96.42% <ø> (ø)
beatx/store/memcached.py 0% <0%> (ø) ⬆️
beatx/tests/test_schedulers.py 100% <100%> (ø) ⬆️
beatx/tests/test_store.py 92.85% <100%> (ø) ⬆️
beatx/tests/utils.py 100% <100%> (ø) ⬆️
beatx/schedulers.py 93.75% <91.42%> (-2.55%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5d42cb...961ea82. Read the comment docs.

@BillSchumacher
Copy link
Contributor Author

Should resolve #3 now, this threw me off for a moment as well.

@BillSchumacher
Copy link
Contributor Author

Should probably note this will break old configurations. Config needs to be updated to: celery_app.config_from_object({
# ...
'beat_scheduler': 'beatx.schedulers.BeatXScheduler',
'beatx_store': 'redis://127.0.0.1:6379/',
# ...
})

Config using celeryconfig.py obj::

CELERYBEAT_SCHEDULER = 'beatx.schedulers.BeatXScheduler'
BEATX_STORE = 'redis://127.0.0.1:6379/

@mixkorshun
Copy link
Owner

Hi!
Sorry for late answering and again thanks for your contribution.

I've close reviewed your changes and write some comments. It should be fixed before I can merge your changes. Also some changes have to be made too:

  • Use interactive rebase (git rebase -i HEAD~32) to correctly squash your changes to only 1-3 commits. (read more https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History). In short you need to squash your changes and force push it to your fork master, merge request will pick it up.
  • You have to do not break backward compatibility with this changes. Python 2 also support named imports, am I right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot set beatx_store with django
4 participants