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

Improved group_name and channel_name validation error messages. #1232

Merged
merged 1 commit into from
Sep 14, 2022

Conversation

ericls
Copy link
Contributor

@ericls ericls commented Jan 26, 2019

This is to include the maximum length in the error message when validating group and channels names.
Also found that invalid_name_error is defined but never used. This PR also utilizes that to format error messages.
Some tests were added.

Closes #1229.

Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ericls.

Thanks for your input here. Are you up for finishing this PR off? (Please let us know. 🙂)

There's a few problems with the new MAX_NAME_LENGTH variable.

  • Nothing fails if I change it to say 200, or 50. (Or any other value.)
  • Surely it needs to be doing something in match_type_and_length()?
  • It should probably be moved to an attribute of BaseChannelLayer so it can be overridden in a subclass.
  • Yes, it should be documented as @jpic notes.

I'm not sure I mind the tests as you've written them. (Yes, testing format() seems unnecessary.) We can come back to that if you resolve the other issues though.

No need to adjust the existing error handling in this PR. Raising the TypeError is fine.

@ericls
Copy link
Contributor Author

ericls commented Feb 4, 2019

Hi @carltongibson and @jpic,
Sorry for the delay and thanks for the review and suggestions.
I do plan to finish this PR by the end of tomorrow (eastern time)

@ericls ericls force-pushed the group-name-error-msg branch from b8339ba to 324d9d7 Compare February 6, 2019 03:20
Base automatically changed from master to main March 3, 2021 18:20
@carltongibson
Copy link
Member

This needs to be rebased after #1792

@ericls ericls force-pushed the group-name-error-msg branch from 324d9d7 to dd94963 Compare September 4, 2022 19:50
@ericls
Copy link
Contributor Author

ericls commented Sep 4, 2022

This needs to be rebased after #1792

Thanks for the reminder, I've rebased the changes on top of the latest main. I've also noticed that the current error message is missing a .format() call, and would throw TypeError: 'str' object is not callable

@carltongibson carltongibson changed the title improve group_name and channel_name validation error message(#1229) Improved group_name and channel_name validation error messages. Sep 14, 2022
Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Thanks @ericls. 👍

Verified

This commit was signed with the committer’s verified signature. The key has expired.
lucasfernog Lucas Fernandes Nogueira
@carltongibson carltongibson merged commit 6ad30e5 into django:main Sep 14, 2022
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.

valid_group_name throwing wrong exception message
4 participants