-
Notifications
You must be signed in to change notification settings - Fork 5.9k
Add new constants #2147
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
Add new constants #2147
Conversation
Bibo-Joshi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
I did a bit of nitpicking. Please add the is_* thingies to Chat, too. Also, we'll need some unittests. Have a look at tests/test_{user/chat}.py some inspiration :) if you want to run one one single test for checking, if it works, you can use pytest -k test_function_name.
Finally, I feel like it would be cleaner to make the is_* thingies properties instead of attributes. While no one in the right mind would change the chats id after initialization, properties could still catch that and also that way it would be clearer that it's just a convenience thingy. So IMHO it should be something like this.
Bibo-Joshi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that looks good :) I left some minor reformulation suggestions. But I'd like @Poolitzer to review anyway, so he can decide about those.
Co-authored-by: Bibo-Joshi <hinrich.mahler@freenet.de>
Co-authored-by: Bibo-Joshi <hinrich.mahler@freenet.de>
Co-authored-by: Bibo-Joshi <hinrich.mahler@freenet.de>
Poolitzer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left two minor comments, thanks for the improvement!
|
Thanks for the contribution! |
closes #2104