-
Notifications
You must be signed in to change notification settings - Fork 47
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
Support django-channels 2 - this PR is looking for someone to take over and finish the work #85
Conversation
Codecov Report
@@ Coverage Diff @@
## master #85 +/- ##
==========================================
- Coverage 75.95% 72.02% -3.94%
==========================================
Files 26 26
Lines 549 579 +30
==========================================
Hits 417 417
- Misses 132 162 +30
Continue to review full report at Codecov.
|
Hi friend! Thanks for the PR! I'm not going to be able to review it, though, because the diff is a bit of a mess :) It seems you are PR'ing against the Thanks :) |
Okay, I've changed the base branch, that seems to work -- @yevheniidehtiar could you confirm if the diff looks as intended? |
Hi, yes the diff looks right 👍 Thank you man) |
@yevheniidehtiar would you be able to resolve the conflicts, and then we can check if tests are running? |
@yevheniidehtiar A lot of great work here, looking forward to seeing it merged :) Could you start by merging the current master of this repository into your branch? This will lead to some merge conflicts that I don't think look very complicated. I'm a bit hesitant to make changes in your branch since it's your |
Ok, man. When I will have free time for this. I merge and create new pull request with my new updates, like messaging over websocket between two clients and bug-fixes with async methods. |
Was re-reading the salutations in this PR 😄 Since today is indeed the International Men's Day, I thought to just quick ask and encourage that your work on this isn't forgotten - but that you are most welcome to resolve the conflicts and we'll get this in! 🙏 |
# Conflicts: # django_nyt/__init__.py # django_nyt/consumers.py # django_nyt/utils.py
Hi guys, seems i forgot it couple years ago and gonna to finish it soon) |
That sounds great, am hear to help resolve decisions and get things merged, much appreciated to have this PR finished 👍 |
Hi @yevheniidehtiar 👋 Time flies :) A lot of work went into this, I was wondering if you could share some background on this PR? Do you have a project that's already running this code? And if so, does it work well? |
Hi @benjaoming Yeap, time flies! Children grows :) Django is already version 3 and soon 4. Have no visible space in the agenda. It might be that it's gonna be updated again when I got a reason to use it. |
@yevheniidehtiar yes, I recognize this time issue very well :) It's nice to hear that the code is actually in a production setting, so without having reviewed it, I can assume that it's useful. I think we should keep the PR open for someone else to notice. |
Closed as you already updated to channels 4 :) |
I want to share upgraded version that work with channels 2.
Sorry, It's my first pull request and I am easy in license and, can u help me with it. Maybe append it reuqest as new branch.