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

Support django-channels 2 - this PR is looking for someone to take over and finish the work #85

Closed
wants to merge 10 commits into from

Conversation

yevheniidehtiar
Copy link

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.

@codecov-io
Copy link

codecov-io commented Mar 9, 2019

Codecov Report

Merging #85 into master will decrease coverage by 3.93%.
The diff coverage is 1.72%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
django_nyt/routing.py 0% <0%> (ø) ⬆️
django_nyt/consumers.py 0% <0%> (ø) ⬆️
django_nyt/subscribers.py 0% <0%> (ø) ⬆️
django_nyt/__init__.py 100% <100%> (ø) ⬆️

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 e6dadb7...bd62c59. Read the comment docs.

@benjaoming
Copy link
Member

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 releases/1.0.x which is a stable release branch, i.e. new features wouldn't go into this branch, anyways. Could you perhaps have a look at the master branch where new development should take place?

Thanks :)

@benjaoming benjaoming changed the base branch from releases/1.0.x to master May 8, 2019 14:39
@benjaoming
Copy link
Member

Okay, I've changed the base branch, that seems to work -- @yevheniidehtiar could you confirm if the diff looks as intended?

@yevheniidehtiar
Copy link
Author

Hi, yes the diff looks right 👍 Thank you man)

@benjaoming
Copy link
Member

@yevheniidehtiar would you be able to resolve the conflicts, and then we can check if tests are running?

@benjaoming
Copy link
Member

@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 master branch and I assume you might be using it for something?

@yevheniidehtiar
Copy link
Author

yevheniidehtiar commented Sep 3, 2019

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.

@benjaoming
Copy link
Member

benjaoming commented Nov 19, 2019

Thank you man
Ok, man

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! 🙏

@yevheniidehtiar
Copy link
Author

Hi guys, seems i forgot it couple years ago and gonna to finish it soon)

@benjaoming
Copy link
Member

That sounds great, am hear to help resolve decisions and get things merged, much appreciated to have this PR finished 👍

@benjaoming
Copy link
Member

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?

@yevheniidehtiar
Copy link
Author

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.
I had a project in 2019 where this code was working very well.

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.

@benjaoming
Copy link
Member

@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.

@benjaoming benjaoming changed the title support django-channels 2 Support django-channels 2 - this PR is looking for someone to take over and finish the work May 3, 2023
@oscarmcm oscarmcm linked an issue May 3, 2023 that may be closed by this pull request
@yevheniidehtiar
Copy link
Author

Closed as you already updated to channels 4 :)

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.

Channels 2
3 participants