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

Add dunder method docstring #3929

Merged
merged 12 commits into from Dec 10, 2023
Merged

Add dunder method docstring #3929

merged 12 commits into from Dec 10, 2023

Conversation

clot27
Copy link
Member

@clot27 clot27 commented Oct 13, 2023

Closes #3926

@clot27 clot27 marked this pull request as draft October 13, 2023 08:49
@clot27 clot27 changed the title [WIP] Add dunder method docstring Add dunder method docstring Oct 30, 2023
@clot27 clot27 marked this pull request as ready for review October 30, 2023 12:22
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 nice PR! I left some comments below :)

One (unrelated) thing that I noticed is that Bot.__eq__ doesn't comply with the documentation of TelegramObject.__eq__, i.e. it doesn't fall back to super().__eq__. Since so for Bot.__eq__ is not explicitly documented, it might be wise to adjust this before adding documentation would make this a (very minor) breaking change. What do you think?
cc @Poolitzer @harshil21

telegram/ext/filters.py Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/_jobqueue.py Outdated Show resolved Hide resolved
telegram/error.py Outdated Show resolved Hide resolved
telegram/error.py Outdated Show resolved Hide resolved
telegram/error.py Outdated Show resolved Hide resolved
telegram/_bot.py Outdated Show resolved Hide resolved
telegram/_bot.py Outdated Show resolved Hide resolved
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 updates! Just two smaller comments below :) What dou you think about #3929 (review) ?

telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/_jobqueue.py Outdated Show resolved Hide resolved
Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

Since so for Bot.eq is not explicitly documented, it might be wise to adjust this before adding documentation would make this a (very minor) breaking change. What do you think?

I agree, should be updated, though we will need to add an _id_attrs for Bot as well, what should that include?

Also telegram.ext.updater.rst still has the :special-members: in it, should be deleted.

Also I believe that a side effect of adding the special-members derivative in the conf.py file was that now we have all these extra documented methods (see BaseFilter docs):
image

telegram/ext/_defaults.py Outdated Show resolved Hide resolved
telegram/ext/_jobqueue.py Outdated Show resolved Hide resolved
telegram/ext/_jobqueue.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Show resolved Hide resolved
@Bibo-Joshi
Copy link
Member

Since so for Bot.eq is not explicitly documented, it might be wise to adjust this before adding documentation would make this a (very minor) breaking change. What do you think?

I agree, should be updated, though we will need to add an _id_attrs for Bot as well, what should that include?

_id_attrs unfortunately don't work for the Bot class, as we're comparing with the bot propery. However, that's only cached once Bot.initialize is called, so not on __init__. Replacing return False with return super().__eq__(other) (and adjusting tests) would be enough IMO.

Also telegram.ext.updater.rst still has the :special-members: in it, should be deleted.

good catch 👍

Also I believe that a side effect of adding the special-members derivative in the conf.py file was that now we have all these extra documented methods (see BaseFilter docs): image

adding object to :inherited-members in filters.rst should exlude these :)

@Bibo-Joshi
Copy link
Member

Also I believe that a side effect of adding the special-members derivative in the conf.py file was that now we have all these extra documented methods (see BaseFilter docs):

adding object to :inherited-members in filters.rst should exlude these :)

I see that there is also __weakref__ in Bot :/ This is getting messier than I thought and it's also somewhat hard to catch … at least with 10min trial & error I didn't find a very good solution yet …

tests/test_bot.py Outdated Show resolved Hide resolved
@Bibo-Joshi Bibo-Joshi merged commit fd6a0fe into master Dec 10, 2023
23 checks passed
@Bibo-Joshi Bibo-Joshi deleted the docstring-dunder-methods branch December 10, 2023 19:17
@github-actions github-actions bot locked and limited conversation to collaborators Dec 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Documentation] Dunder methods
3 participants