Bot api 3.2 by jsmnbom · Pull Request #732 · python-telegram-bot/python-telegram-bot

jsmnbom · 2017-07-21T19:29:19Z

Closes #730

Note the lack of proper tests for StickerSet... it's hard test creation and such when we can't delete them afterwards.

Every bot method is documented as per new standard in #728. I have not added or changed the object docs for Sticker, StickerSet and MaskPosition however since that would potentially conflict with #728, so it's easier to just leave that to @Eldinnie :)

* get_sticker_set * upload_sticker_file * create_new_sticker_set * add_sticker_to_set * set_sticker_position_in_set * delete_sticker_from_set

Change it so it works, but add a todo notice about it Also add missing **kwargs so it's for robust in future

NOTE: Currently not testing StickerSet in terms of bot methods (interaction with telegrams servers)

thodnev

Not a full review, just a quick look
Looks good to me, but I've not tested it.

thodnev · 2017-07-21T19:38:17Z

+        if not data:
+            return list()
+
+        stickers = list()


It's better to use comprehensions etc here like:
stickers = [Sticker.de_json(ix, bot) for ix in data]

Hmm, you're probably right, I just wrote it like we had in other places. I'll change it though.

thodnev · 2017-07-21T19:41:39Z

+
+    @staticmethod
+    def de_json(data, bot):
+        if not data:


Probably we need to raise an exception here or make it return empty stickerset. Exception looks better in my opinion

Nah, it's never been needed before in any of the other de_json, so we should be 100% fine

i agree with @bomjacob

thodnev · 2017-07-21T19:44:17Z

+    MOUTH = 'mouth'
+    CHIN = 'chin'
+
+    def __init__(self, point, x_shift, y_shift, zoom, **kwargs):


If it's used mainly for storage it's better to use slots

Hmm. We don't use slots anywhere else, should we?

constants all over the lib. Suggesting same approach here.

Eldinnie

Mostly docstring stuff.

One important thing I ask is to remove **kwargs from the bot methods and their docstrings.

Eldinnie · 2017-07-21T21:01:32Z

+            **kwargs (:obj:`dict`): Arbitrary keyword arguments.
+
+        Returns:
+            :class:`telegramStickerSet`


Missing a . here (telegram.StickerSet)

Eldinnie · 2017-07-21T21:02:14Z

+    def upload_sticker_file(self, user_id, png_sticker, timeout=None, **kwargs):
+        """
+        Use this method to upload a .png file with a sticker for later use in
+        :attr:`create_new_sticker_set` and :attr:`add_sticker_to_set methods` (can be used multiple


closing ``` is after methods, should close at _set

Eldinnie · 2017-07-21T21:02:41Z

+            ``open(filename, 'rb')``
+
+        Args:
+            user_id (int): User identifier of sticker file owner.


Eldinnie · 2017-07-21T21:19:02Z

+
+        url = '{0}/getStickerSet'.format(self.base_url)
+
+        data = {'name': name}


Definging **kwargs in arguments but not adding them to data is useless. I suggest removing them from arguments.

Good catch @Eldinnie
kwargs are automatically passed when using the @message decorator, but if it's not being used, than they should be pushed in the data dictionary.
the reason for that, is to allow forward compatibility (lets say telegram adds some arguments t an existing API call, then users can use the new API even before we release an official support).

anyway, we can fix that in another PR as it's not the only place where we missed that.

Eldinnie · 2017-07-21T21:19:38Z

+
+        url = '{0}/uploadStickerFile'.format(self.base_url)
+
+        data = {'user_id': user_id, 'png_sticker': png_sticker}


same note about **kwargs.

Eldinnie · 2017-07-21T21:26:09Z

+            **kwargs (:obj:`dict`): Arbitrary keyword arguments.
+
+        Returns:
+            :obj:`bool` On success, ``True`` is returned.


Eldinnie · 2017-07-21T21:26:25Z

+        Use this method to delete a sticker from a set created by the bot.
+
+        Args:
+            sticker (:obj:`str`): File identifier of the sticker


Eldinnie · 2017-07-21T21:26:31Z

+            timeout (:obj:`int` | :obj:`float`, optional): If this value is specified, use it as
+                the read timeout from the server (instead of the one specified during
+                creation of the connection pool).
+            **kwargs (:obj:`dict`): Arbitrary keyword arguments.


Eldinnie · 2017-07-21T21:26:36Z

+            **kwargs (:obj:`dict`): Arbitrary keyword arguments.
+
+        Returns:
+            :obj:`bool` On success, ``True`` is returned.


Eldinnie · 2017-07-21T21:28:12Z

+    MOUTH = 'mouth'
+    CHIN = 'chin'
+
+    def __init__(self, point, x_shift, y_shift, zoom, **kwargs):


constants all over the lib. Suggesting same approach here.

Added new style docstrings for: Sticker StickerSet MaskPosition

jsmnbom · 2017-07-22T09:21:45Z

The failed build is due to a possible bug in telegrams documentation. I've already contacted botsupport. Should I add an overwrite so our builds pass or do we wanna wait for a reply?

jsmnbom added 9 commits July 21, 2017 19:04

Add set_name and mask_position to Sticker

9801772

Add new sticker types; StickerSet and MaskPosition

d4ee12c

Add new sticker types to init

bb863dd

Add new sticker methods to bot

792546d

* get_sticker_set * upload_sticker_file * create_new_sticker_set * add_sticker_to_set * set_sticker_position_in_set * delete_sticker_from_set

StickerSet.is_masks should be plural

acd0dc8

Allow png_stickers to be sent as files

763abad

is_masks is called contains_masks by telegrams servers!

9262c62

Change it so it works, but add a todo notice about it Also add missing **kwargs so it's for robust in future

Add tests for new sticker stuff

79db6d1

NOTE: Currently not testing StickerSet in terms of bot methods (interaction with telegrams servers)

Add to_dict to StickerSet

ac0fd6a

thodnev reviewed Jul 21, 2017

View reviewed changes

Eldinnie requested changes Jul 21, 2017

View reviewed changes

Update new sticker stuff as per reviews

073fb4f

Eldinnie approved these changes Jul 21, 2017

View reviewed changes

Added docstring

247ba69

Added new style docstrings for: Sticker StickerSet MaskPosition

tsnoam merged commit 5a37af6 into master Jul 22, 2017

tsnoam deleted the bot-api-3.2 branch July 22, 2017 11:35

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


		url = '{0}/getStickerSet'.format(self.base_url)

		data = {'name': name}


		url = '{0}/uploadStickerFile'.format(self.base_url)

		data = {'user_id': user_id, 'png_sticker': png_sticker}

Uh oh!

Conversation

jsmnbom commented Jul 21, 2017

Uh oh!

thodnev 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!

Eldinnie 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!

jsmnbom commented Jul 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants