Add warning for the length of the group name #2122
Conversation
There was a problem hiding this comment.
Hi @IronJam11. Thanks for this. Needs a regression test, demonstrating the fix.
channels/layers.py
Outdated
| @@ -201,6 +205,7 @@ async def flush(self): | |||
| raise NotImplementedError("flush() not implemented (flush extension)") | |||
|
|
|||
| async def group_add(self, group, channel): | |||
| print("Hello") | |||
There was a problem hiding this comment.
This (and others) need removing.
|
I will work on it. thank you for your review. if you could elaborate on regression test it will be helpful for me as I am a bit new to this org |
|
Look at the existing test suite for examples, I'd suggest. https://channels.readthedocs.io/en/latest/contributing.html#how-do-i-get-started-and-run-the-tests |
|
I have successfully tested it on my local, there are issues with the tests. :) And I removed all the unnecessary statements as well |
|
Correct me if I am wrong, so i ran pytest (all tests passed with 2 warning only), does that qualify to be fit for a valid pr? |
|
@IronJam11 You need to add a new test showing the issue. It should fail without your patch and pass with it. Django has a tutorial for writing your first patch: https://docs.djangoproject.com/en/dev//intro/contributing/#writing-some-tests-for-your-ticket It's not exactly the same here, but the general ideas are the same. |
|
I have added a regression test as mentioned. Is there anything else I will have to attach? |
|
@IronJam11 it looks like you've REPLACED an existing test with your new test, we don't want to lose the old one! |
|
Apologies, I added the erased test |
|
Looking close @IronJam11 — thanks. I'm bemused by the existing (Not an issue with this PR @IronJam11!) |
channels/layers.py
Outdated
| assert self.valid_group_name(group), ( | ||
| f"Group name must be" f"less than {self.MAX_NAME_LENGTH} characters." | ||
| ) |
There was a problem hiding this comment.
As per the point about the TypeError being raised before the AssertionError, it's not clear we need to make any change here, or on lines 344/349 and 353/360 above.
|
You are absolutely right ! My bad there (I had actually used it to debug initially), i ran the tests again and it seems to be working just fine :) |
There was a problem hiding this comment.
So the assert error message is never needed. 🤔 @bigfootjon Am I just misreading that?
Lol, yeah you're reading that right. We should probably remove the assert altogether and change the method name to require_valid_group_name so that behavior is more clear.
|
I inlined it as well :), also other than this, if there are other issues to tackle, I will be more than happy to contribute :} |
…ts whenever necessary
I made the necessary changes |
channels/layers.py
Outdated
| assert self.valid_group_name(group), "Group name not valid" | ||
| assert self.valid_channel_name(channel), "Channel name not valid" | ||
| self.require_valid_group_name(group) | ||
| self.valid_channel_name(channel), "Channel name not valid" |
There was a problem hiding this comment.
Can you rename valid_channel_name to require_valid_channel_name to match?
And also remove the , "Channel name not valid" since it isn't used?
channels/layers.py
Outdated
| assert self.valid_group_name(group), "Group name not valid" | ||
| assert self.valid_channel_name(channel), "Channel name not valid" | ||
| self.require_valid_group_name(group) | ||
| self.valid_channel_name(channel), "Channel name not valid" | ||
| # Add to group dict | ||
| self.groups.setdefault(group, {}) | ||
| self.groups[group][channel] = time.time() | ||
|
|
||
| async def group_discard(self, group, channel): | ||
| # Both should be text and valid | ||
| assert self.valid_channel_name(channel), "Invalid channel name" |
|
channels/layers.py
Outdated
|
|
||
| assert all( | ||
| self.valid_channel_name(channel, receive=receive) for channel in names | ||
| all( |
There was a problem hiding this comment.
The all doesn't have semantic value, can you refactor this as a normal for loop?
|
Done as you said :) |
There was a problem hiding this comment.
@carltongibson any final thoughts?
@IronJam11 please fix the lint errors as indicated in the failing signal on this PR
|
sorry sir, done !! |
channels/layers.py
Outdated
| return True | ||
| for channel in names: | ||
| self.require_valid_channel_name(channel, receive=receive) | ||
| return |
There was a problem hiding this comment.
Do we mean return True? return None (explicitly)? Which?
|
yeah absolutely it is supposed to be return True. |
Resolves #1845
Corresponding screenshot :-