Skip to content

Conversation

@NikitaPirate
Copy link
Contributor

@NikitaPirate NikitaPirate commented Oct 15, 2020

closes #2104

Copy link
Member

@Bibo-Joshi Bibo-Joshi left a 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.

Copy link
Member

@Bibo-Joshi Bibo-Joshi left a 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.

@Bibo-Joshi Bibo-Joshi requested a review from Poolitzer October 15, 2020 20:22
NikitaPirate and others added 4 commits October 15, 2020 23:43
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>
Copy link
Member

@Poolitzer Poolitzer left a 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!

@Bibo-Joshi Bibo-Joshi merged commit 165a24e into python-telegram-bot:master Oct 18, 2020
@Bibo-Joshi
Copy link
Member

Thanks for the contribution!

@github-actions github-actions bot locked and limited conversation to collaborators Oct 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Overhaul & extension of constants

3 participants