Docs update by Eldinnie · Pull Request #728 · python-telegram-bot/python-telegram-bot

Eldinnie · 2017-07-19T01:10:51Z

So here it is. All new and unified docs.
There might be some small spelling errors (if you find them please notify me).
It's a moloch of a pull request but I already included latest master (and updated the docstrings for new methods).

WebhookInfo User UserProfilePhotos WebhookInfo Update User UserProfilePhotos WebhookInfo ReplyKeyboardRemove ReplyKeyboardRemove small typo ReplyKeyboardMarkup MessageEntity Message KeyboardButton ForceReply Errors Constants ChosenInlineResult ChatMember ChatAction MessageEntity.rst Chat CallbackQuery Base

unicode characters Copy paste from bot api website copied some unicode characters that made tests fail on python 2.7 Remove type from user tests

Animation CallbackGame Game GameHighscore

Invoice LabeledPrice OrderInfo PreCheckoutQuery ShippingAddress ShippingOption ShippingQuery SuccessfulPayment

InlineKeyboardButton InlineKeyboardMarkup InlineQuery InlineQueryResult InlineQueryResultArticle InlineQueryResultAudio InlineQueryResultArticle InlineQueryResultAudio InlineQueryResultCachedAudio InlineQueryResultCachedDocument InlineQueryResultCachedGif InlineQueryResultCachedMpeg4Gif InlineQueryResultCachedPhoto InlineQueryResultCachedSticker InlineQueryResultCachedVideo InlineQueryResultCachedVoice InlineQueryResultContact InlineQueryResultDocument InlineQueryResultGame InlineQueryResultGif InlineQueryResultLocation InlineQueryResultMpeg4Gif InlineQueryResultPhoto InlineQueryResultVenue InlineQueryResultVideo InlineQueryResultVoice InputContactMessageContent InputLocationMessageContent InputMessageContent InputTextMessageContent InputVenueMessageContent

Audio Contact Document File InputFile Location PhotoSize Sticker Venue Video VideoNote Voice Files

CommandHandler InlineQueryHandler MessageHandler

ChosenInlineResultHandler PreCheckoutQueryHandler Regexhandler - Also modified the check_update part for this. TypeHandler StringRegexHandler StringCommandHandler ShippingQueryHandler Handler ConversationHandler

Updated all new docs in new style

Filters Dispatcher JobQueue Updater MessageQueue

rahiel · 2017-07-19T09:31:19Z

In #726 I added a note on where to find the max message length in bot.send_message, this is absent from this PR.

Eldinnie · 2017-07-19T09:40:47Z

@rahiel Because Max message length is now in the docstring for every text message method

Eldinnie · 2017-07-19T11:55:05Z

Regarding the questions @bomjacob asked:

When we have an arg like: Arg (type): Text. Should Text then be Text or text (aka start with a capital letter)?. It's not quite consistent (and fairly easy to fix with a regex replace if we just decide on a preferred style.
A: IMO yes, always capitals at the start of sentences. I'll run a check I thought I had them all.
Is there any way to inherit docstrings? (thinking about handlers and such, which have a lot of repetition in their handle_ and check_ and such)
A: There might be. But atm we've got them all covered.
Should we give all the callbacks a set type too? (if it's even possible?) Typing using Callable[[Arg1Type, Arg2Type], ReturnType] but I'm not sure sphinx understands that.
A: Function is also a type (or maybe change to callable).
There are ALOT of places where we have args and attributes that are almost exactly the same... Could we figure out a better way to write that? Cause I don't think it looks very good to just have everything be copy pasted everywhere... Maybe like use args if the user is meant to instantiate it themselves only?
A: The docs describe the objects. And for most of our objects, it's attributes are the same as their constructing arguments. Hence they're both in the docs. Pro is that the roolsbot can link to attributes, but not to arguments. I tried to make the attributes more descriptive of what it is, and the arguments more defining what is expected when constructing. Maybe I can edit this more to make the attributes more compact.
According to napoleon's google docstring format example optional arguments should be like: param2 (:obj:'int', optional): Description of 'param2'. Multiple lines are supported. and not param2 (Optional[:obj:'int']): Description of 'param2'. Multiple lines are supported. like we're using atm. (just pretend those ' are ... github is being annyoing.) Should we fix that? (I personally like the first style better) (if we decide on the latter we should figure out of the o in optional should be capital or not) A: I followed the way it was done in bot.py` (Optional[str]). I have no problem converting it to the styleguide. A lot of extra work though
Also in line with the above, is there a convention for when to use :obj'int' and when to use just int? It seems we don't use the :obj: notation anywhere? Should we?
A: The style guide give both options. It doesn't really matter much as long as it is consequent. I think I made it consequent.
It also uses :obj:'list' of :obj:'str' instead of List[str]. I personally think that in this case the latter looks a whole lot better. But I'm not sure if napoleon parses it differently?
A: I changed all List[str] to list(str) and napoleon parses them fine. It looks to most natural and pythonic to me.
After a while I stopped commenting about square brackets. (I've since removed those comments in favour of this note.) Is there a reason you're replacing them all? I'm not necessarily against it, I just want a reason since I personally think square brackets look better. It also seems a bit inconsistent between args and attributes sections (see InlineKeyboardMarkup). It seems you use [] to signify a list in attributes section but not in args?
A: Also see answer in 7, but in attributes brackets are parsed differently than in arguments. I tried to find a style that would look best and readable on both sections. Also I made it consequent. But again a good overhaul to the style guide might be best option.

jsmnbom · 2017-07-19T14:07:35Z

:D
Alright, this works for now then.
Yeah maybe change to callable :)
Hmm, I guess it's fine for now... just felt that there was a lot of bulk when you look through the code, which I'm not a fan of :/
I personally prefer the styleguide way of doing things... We should probably ask others though. @python-telegram-bot/developers Any preferences? I'm honestly not even sure where the Optional[type] notation comes from :/
Alright sounds good :)
and 8.
This is where we disagree :) I prefer we used much the same notation as is used in typing since that seems like the "official" python way to do type declarations to me. This means using List[type] and Tuple[type]. Once again asking @python-telegram-bot/developers :)
However with that being said, I think using parenthesis looks alright too. I do not like how the syntax is different between the Args and Attributes sections... Imo, if we need to have them both, they should most certainly have the same type declarations. It's not very user (as in us developers) friendly to have square brackets mean 2 different things (Optional[type] and [str], first one meaning it's optional and the second one meaning it's a list).

Eldinnie · 2017-07-19T14:36:30Z

I wasn't know with the typing stuff. Now I've seen it I aggree on 7 and 8. Will make try how it looks.

All classes and methods now use the google style guide for docstrings.

jsmnbom

Okay that should be my last comments as far as docstring and code goes (haven't looked at rst files since you said those weren't done yet.

I know it looks like a butt-ton of comments, but most of them are fairly trivial :)

jsmnbom · 2017-07-22T16:14:15Z

+            chat_id (:obj:`int` | :obj:`float`): Unique identifier for the target chat or username
+                of the target channel (in the format @channelusername).
+            text (:obj:`str`): Text of the message to be sent. Max 4096 characters. Also found as
+                ``telegram.constants.MAX_MESSAGE_LENGTH``.


:const:`telegram.constants.MAX_MESSAGE_LENGTH`
(also found elsewhere, search for "4096")

jsmnbom · 2017-07-22T16:15:21Z

-                @channelusername).
-            message_id (int): Unique message identifier.
-            timeout (Optional[int|float]): If this value is specified, use it as the read timeout
+            chat_id (:obj:`int` | :obj:`float`): Unique identifier for the target chat or username


I think you mean :obj:`int` | :obj:`str`
(also applies elsewhere)

jsmnbom · 2017-07-22T16:21:52Z

-        """As of v.4.0, Telegram clients support rounded square mp4 videos of up to 1 minute
-        long. Use this method to send video messages
+        """
+        As of v.4.0, Telegram clients support rounded square mp4 videos of up to 1 minute long.


The first line should describe what the method does. Either flip around the lines or in this case simply remove it.

jsmnbom · 2017-07-22T16:23:25Z

+            action(:class:`telegram.ChatAction` | :obj:`str`): Type of action to broadcast. Choose
+                one, depending on what the user is about to receive:
+
+                * typing for text messages


The constants are preferred here I'd say

The method doesn't care, so I put it this way.

I would very very much like to encourage our users to use the constants though

Changed doc to remove the list from this method and point to ChatAction

jsmnbom · 2017-07-22T16:24:30Z

        if isinstance(other, self.__class__):
            return self._id_attrs == other._id_attrs
-        return super(TelegramObject, self).__eq__(other)
+        return super(TelegramObject, self).__eq__(other)  # pylint: disable=E1101


Could you maybe use the new named pylint names instead of the id?
# pylint: disable=no-member

imo out of scope.

Out of scope of this PR? You added the comment though... I'm not asking you to go back and find all our pylint lines, I'm just thinking that we should start slowly switching over so we make it easier for ourselves in the future.

you're right (forgot I put this in)

jsmnbom · 2017-07-22T18:28:18Z

    def __init__(self,
                 id,
                 first_name,
-                 type=None,


This means that we break backwards compat doesn't it?

users are created by tg which does not send type. So I guess not

We still technically do though... @tsnoam thoughts?

It does not break tests and users have no type, so I see no problem in just removing it. It's still captured with **kwargs if needed.

jsmnbom · 2017-07-22T18:29:47Z

    Handler class to handle updates of custom types.

+    Attributes:
+        type (:obj:`class`): The ``type`` of updates this handler should process.


Shouldn't that be of type type?

It's a bit of a weird handler. You use it like:

import InlineQuery add_handler(TypeHandler(InlineQuery))

Exactly you pass it a class which has the type type

>>> class Test: ... pass ... >>> type(Test) <class 'type'> >>>

Oh! ok, will change

jsmnbom · 2017-07-22T18:31:25Z

+    Args:
+        latitude (:obj:`float`): Latitude of the location in degrees.
+        longitude (:obj:`float`): Longitude of the location in degrees.
+        title (:obj:`str`); Name of the venue.


Semi colon?
(also applies elsewhere (a search for ; is fairly quick)

jsmnbom · 2017-07-22T18:32:09Z

+        selective (:obj:`bool`, optional): Use this parameter if you want to force reply from
+            specific users only. Targets:
+
+            1) users that are @mentioned in the text of the Message object;


Remove semi colon

jsmnbom · 2017-07-22T18:33:41Z

-        shipping_query (:class:`telegram.ShippingQuery`): New incoming shipping query.
-        pre_checkout_query (:class:`telegram.PreCheckoutQuery`): New incoming pre-checkout query.
+    Note:
+        At most one of the optional parameters can be present in any given update.


"With the exception of update_id which is always present."

that's why it's "at most one of the optional parameters..."

Eldinnie · 2017-07-22T23:21:46Z

I addressed all matter @bomjacob mentioned. IMO it's ready for merge @tsnoam or @jh0ker

jsmnbom · 2017-07-23T12:47:47Z


    Args:
-        callback (:obj:`callable): A function that takes ``bot, update`` as positional arguments.
+        callback (:obj:`callable'): A function that takes ``bot, update`` as positional arguments.


Is that a ' instead of `?

jsmnbom · 2017-07-23T12:49:17Z


    Note:
-        You must use exactly one of the optional fields.
+        You must use exactly one of the optional fields. Mind that :attr:`callback_game` is not


This isn't true though. callback_game the attribute is implemented, but it just accepts a games short name I think, otherwise there would be no way to launch games from custom keyboards.

restored old order of kwargs in regexhandler added deprecation warning

Parameter to StickerSet was flasely named on TG's docs as is_masks. It's fixed now.

Conflicts: CHANGES.rst telegram/base.py telegram/bot.py telegram/callbackquery.py telegram/chat.py telegram/chatmember.py telegram/choseninlineresult.py telegram/files/audio.py telegram/files/chatphoto.py telegram/files/contact.py telegram/files/document.py telegram/files/file.py telegram/files/location.py telegram/files/photosize.py telegram/files/sticker.py telegram/files/video.py telegram/files/videonote.py telegram/files/voice.py telegram/forcereply.py telegram/games/animation.py telegram/games/game.py telegram/games/gamehighscore.py telegram/inline/inlinekeyboardbutton.py telegram/inline/inlinekeyboardmarkup.py telegram/inline/inlinequery.py telegram/message.py telegram/messageentity.py telegram/payment/invoice.py telegram/payment/labeledprice.py telegram/payment/orderinfo.py telegram/payment/precheckoutquery.py telegram/payment/shippingaddress.py telegram/payment/shippingoption.py telegram/payment/shippingquery.py telegram/payment/successfulpayment.py telegram/replykeyboardmarkup.py telegram/replykeyboardremove.py telegram/replymarkup.py telegram/update.py telegram/user.py telegram/userprofilephotos.py telegram/webhookinfo.py

Eldinnie added 12 commits June 26, 2017 23:43

Bot

eae8f5e

Fix tests

cb12419

unicode characters Copy paste from bot api website copied some unicode characters that made tests fail on python 2.7 Remove type from user tests

Games

bf36f8f

Animation CallbackGame Game GameHighscore

Payment

c81ca52

Invoice LabeledPrice OrderInfo PreCheckoutQuery ShippingAddress ShippingOption ShippingQuery SuccessfulPayment

Files

be79d3e

Audio Contact Document File InputFile Location PhotoSize Sticker Venue Video VideoNote Voice Files

CallbackQueryHandler

0a844ea

CommandHandler InlineQueryHandler MessageHandler

Handlers

6cabaa6

ChosenInlineResultHandler PreCheckoutQueryHandler Regexhandler - Also modified the check_update part for this. TypeHandler StringRegexHandler StringCommandHandler ShippingQueryHandler Handler ConversationHandler

Merge 'origin/master' 3.1

51de9a3

Updated all new docs in new style

Ext

7372dd1

Filters Dispatcher JobQueue Updater MessageQueue

added ChatPhoto and VideoNote to toctree

c1b98d6

Eldinnie mentioned this pull request Jul 19, 2017

Fix Filters documentation #570

Closed

Add reference to max message length constant to text sending methods

04aaee6

This was referenced Jul 19, 2017

add docs for bot api 3.0 modules #684

Merged

small docstring updates #726

Closed

Eldinnie force-pushed the docs_update branch from 6b731b9 to 00032c8 Compare July 21, 2017 15:37

jsmnbom mentioned this pull request Jul 21, 2017

Bot api 3.2 #732

Merged

Eldinnie force-pushed the docs_update branch from 00032c8 to 5723be4 Compare July 21, 2017 21:52

styleguide

a442f55

All classes and methods now use the google style guide for docstrings.

Eldinnie force-pushed the docs_update branch from 5723be4 to a442f55 Compare July 22, 2017 15:52

Merge remote-tracking branch 'origin/master' into docs_update

1fd9d76

jsmnbom requested changes Jul 22, 2017

View reviewed changes

Eldinnie added 2 commits July 23, 2017 00:16

structure and prettify docs rst

1767477

Addressing review by @bomjacob

fbf763a

Last comments by @bomjacob addressed

f1e827a

jsmnbom reviewed Jul 23, 2017

View reviewed changes

Eldinnie added the 📋 pending-merge work status: pending-merge label Jul 23, 2017

Eldinnie added 4 commits July 23, 2017 15:42

As discussed with @tsnoam

bf1218f

restored old order of kwargs in regexhandler added deprecation warning

changes.rst

60b5f64

contains_masks

e539c4f

Parameter to StickerSet was flasely named on TG's docs as is_masks. It's fixed now.

contains_masks

f0abcce

tsnoam removed the 📋 pending-merge work status: pending-merge label Jul 23, 2017

Eldinnie and others added 4 commits July 23, 2017 21:51

damngling comma

084c3cb

last changes

0744d7a

Merge remote-tracking branch 'origin/docs_update' into docs_update

685ccc3

jsmnbom mentioned this pull request Jul 23, 2017

The 'latest' version of the readthedocs is empty. #740

Closed

Eldinnie and others added 2 commits July 23, 2017 22:29

Last and final push

50a8829

CHANGES: fix note about Botan

4691a55

tsnoam merged commit b3b9424 into master Jul 23, 2017

tsnoam deleted the docs_update branch July 23, 2017 20:33

github-actions Bot locked and limited conversation to collaborators Aug 23, 2020

Uh oh!

Conversation

Eldinnie commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rahiel commented Jul 19, 2017

Uh oh!

Eldinnie commented Jul 19, 2017

Uh oh!

Eldinnie commented Jul 19, 2017

Uh oh!

jsmnbom commented Jul 19, 2017

Uh oh!

Eldinnie commented Jul 19, 2017

Uh oh!

jsmnbom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eldinnie commented Jul 22, 2017

Uh oh!

jsmnbom Jul 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Eldinnie commented Jul 19, 2017 •

edited

Loading

jsmnbom Jul 23, 2017 •

edited

Loading