Skip to content

Conversation

@clot27
Copy link
Member

@clot27 clot27 commented Jul 3, 2022

Supersedes #3134

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.

Nice work. Left some comments below. If it's not too much work, can you rebase your branch on the top of the doc-fixes branch? If done correctly, Github will only show your commits and remove the pool size commit.

@harshil21 harshil21 added the ⚙️ documentation affected functionality: documentation label Jul 6, 2022
@harshil21 harshil21 added this to the v20.0a3 milestone Jul 6, 2022
@clot27
Copy link
Member Author

clot27 commented Jul 7, 2022

Nice work. Left some comments below. If it's not too much work, can you rebase your branch on the top of the doc-fixes branch? If done correctly, Github will only show your commits and remove the pool size commit.

Umm, harshil I dont know how to rebase branches 🤔 could you help me please?

@Bibo-Joshi
Copy link
Member

this article should get you started :) if you need more help, please ping us in the dev-chat on tg

@clot27
Copy link
Member Author

clot27 commented Jul 8, 2022

Why the codacy test is failing?

@Poolitzer
Copy link
Member

It cant find the git difference for some reason

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.

nice job rebasing, most of my comments is just now moving stuff above the Args section.

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.

Hi. Sorry, I'm a bit late to the review-party … The additions are all nice 👍 I have three more general comments:

  1. CallbackQuery has a bunch of shortcut methods as well, like edit_message_text. Can you check the documentation of that class and also link them in the Bot class?

  2. the User class also has all the send_* shortcuts, like Chat does. please also link these :)

  3. many .. seealso:: directives are in the wrong position in the docstring. The docstring should have the following order:

    1. General description of the class/method
    2. seealso (sometimes this can also be part of 1.)
    3. .. versionadded::/.. versionchanged::
    4. Arguments
    5. Attributes/Returns

    Please move your additions accordingly

I also saw a few added newlines, that we don't really need, but that's not overly important.

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! I left a few nitpick-comments :) In additon, the shortcut methods {User, Chat}.get/set_menu_button can also be linked in Bot.{get, set}_chat_menu_button

by Telegram.
.. seealso:: :attr:`telegram.Message.reply_animation`, :attr:`telegram.Chat.send_animation`
Copy link
Member

Choose a reason for hiding this comment

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

User.send_animation

.. seealso:: :attr:`telegram.Message.copy`, :attr:`telegram.Chat.send_copy`,
:attr:`telegram.Chat.copy_message`, :attr:`telegram.User.send_copy`
Copy link
Member

Choose a reason for hiding this comment

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

User.copy_message

Comment on lines 36 to 38
.. seealso:: `contexttypesbot.py <https://github.com/python-telegram-bot/
python-telegram-bot/blob/master/examples/contexttypesbot.py>`_
Copy link
Member

@Bibo-Joshi Bibo-Joshi Jul 17, 2022

Choose a reason for hiding this comment

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

Suggested change
.. seealso:: `contexttypesbot.py <https://github.com/python-telegram-bot/
python-telegram-bot/blob/master/examples/contexttypesbot.py>`_
.. seealso:: `contexttypesbot.py <examples.contexttypesbot.html>`_

the other links use this - I take it this was tested?

Copy link
Member

Choose a reason for hiding this comment

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

yes

Comment on lines 70 to 71
.. seealso:: `arbitrarycallbackbot.py <https://github.com/python-telegram-bot/
python-telegram-bot/blob/master/examples/arbitrarycallbackdatabot.py>`_,
Copy link
Member

Choose a reason for hiding this comment

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

see above

chats and may not be set for inline queries coming from third-party clients. These
updates won't be handled, if :attr:`chat_types` is passed.
.. seealso:: `inlinebot.py <https://github.com/python-telegram-bot/
Copy link
Member

Choose a reason for hiding this comment

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

s.a.

When setting :paramref:`block` to :obj:`False`, you cannot rely on adding custom
attributes to :class:`telegram.ext.CallbackContext`. See its docs for more info.
.. seealso:: `pollbot.py <https://github.com/python-telegram-bot/
Copy link
Member

Choose a reason for hiding this comment

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

.

When setting :paramref:`block` to :obj:`False`, you cannot rely on adding custom
attributes to :class:`telegram.ext.CallbackContext`. See its docs for more info.
.. seealso:: `pollbot.py <https://github.com/python-telegram-bot/
Copy link
Member

Choose a reason for hiding this comment

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

.

@harshil21 harshil21 changed the title Crosss references Cross references Jul 20, 2022
@Bibo-Joshi Bibo-Joshi merged commit 3d8dc4c into python-telegram-bot:doc-fixes Jul 20, 2022
@Bibo-Joshi
Copy link
Member

Thank you very much for your contribution and the patience during the review process :)

@Bibo-Joshi Bibo-Joshi mentioned this pull request Jul 20, 2022
2 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Jul 28, 2022
@lemontree210
Copy link
Member

addresses #3110 (just adding for this PR to show up on the page of the issue)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

⚙️ documentation affected functionality: documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants