Auto Pagination for answer_inline_query #2072

Bibo-Joshi · 2020-08-27T17:50:46Z

As discussed in #2071.

Closes #2071

Bibo-Joshi · 2020-09-10T13:19:32Z

We might want to allow for pagination with less then 50 items per page. That should be useful for use cases where the computation of each results takes long.

Bibo-Joshi · 2020-09-13T13:44:48Z

We might want to allow for pagination with less then 50 items per page. That should be useful for use cases where the computation of each results takes long.

Actually, in the case results is a callable, that's already covered and that's the interesting case for this use case anyway. I tweaked the tests to make that clear.

JosXa · 2020-09-13T13:51:09Z

telegram/bot.py

                            **kwargs):
        """
        Use this method to send answers to an inline query. No more than 50 results per query are
        allowed.

        Args:
            inline_query_id (:obj:`str`): Unique identifier for the answered query.
-            results (List[:class:`telegram.InlineQueryResult`)]: A list of results for the inline
-                query.
+            results (List[:class:`telegram.InlineQueryResult`] | Callable): A list of results for


Are generic type annotations possible for callable in docstrings?

Callable[[int], Optional[List[:class:`telegram.InlineResult`]]]

?

What happens when user returns empty list?

Clarify

E.g. for callbacks we also just state Callable and state the signature explicitly in the docstring (see Handler). Personally, I don't find Callable[[int], Optional[List[:class:telegram.InlineResult]]] overly readible and newbies will not understand it.

What happens when user returns empty list?

»or :obj:None, if there are no more results.« Is that ambiguous

There's also Protocol:

class ResultsGenerator(typing.Protocol): def __call__(self, current_index: int) -> typing.List[telegram.InlineResult]: ...

But would be quite revolutionary to add that, and even more verbose ;) So just FYI

I don't find Callable[[int], Optional[List[:class:telegram.InlineResult]]] overly readible

Yea, generally that syntax is awful. Takes a bit of getting used to

»or :obj:None, if there are no more results.« Is that ambiguous

Maybe something along the lines of "or an expression that evaluates to False" (bad wording, I know, but you get the gist)

But would be quite revolutionary to add that, and even more verbose ;) So just FYI

good to know :)

Maybe something along the lines of "or an expression that evaluates to False" (bad wording, I know, but you get the gist)

I'd rather leave that as implementation detail. Documenting it might lead to problems, if the implementation needs to be changed and None is easier to graps for newbies

telegram/bot.py

JosXa · 2020-09-13T16:13:39Z

telegram/bot.py

+            if current_offset == '':
+                current_offset = 0
+            else:
+                current_offset = int(current_offset)


Account for negative value with max(current_offset, 0)?

In what case would we have a negative value here?

Defensive coding

IMHO it would just make debugging very hard, if invalid negative input lead to completely unexpected behaivour instead of leading to an exception thrown … I'll add a warning, that current_offset shouldn't usually be passed manually bot InlineQuery.answer(, auto_pagination=True) should take care of it.

telegram/bot.py

JosXa · 2020-09-13T16:20:28Z

telegram/bot.py

+                    next_offset = current_offset + 1
+                    results = results[
+                        current_offset * MAX_INLINE_QUERY_RESULTS:
+                        current_offset * MAX_INLINE_QUERY_RESULTS + MAX_INLINE_QUERY_RESULTS]


This could just use next_offset * MAX, right?

Actually next_offset could be used in a few places here

I see how to use it here, but I don't see where else to use it …

JosXa · 2020-09-13T16:24:17Z

tests/test_bot.py

@@ -385,6 +400,97 @@ def test(_, url, data, *args, **kwargs):
                                               switch_pm_text='switch pm',
                                               switch_pm_parameter='start_pm')

+    def test_answer_inline_query_current_offset_error(self, bot, inline_results):


Looks like this could become a lot more concise using @pytest.mark.parametrize

I don't think I understand you here

https://docs.pytest.org/en/latest/parametrize.html#pytest-mark-parametrize

This generates new tests given a bunch of parameter combinations so that you don't need to make multiple assertions in a single test.

Here's an example for classs-based test method parametrization:

class TestCase: @pytest.mark.parametrize("test_input,expected", [ ("3+5", 8), ("2+4", 6), ("6*9", 42), # fails ]) def test_eval_combinations(self, test_input, expected): assert eval(test_input) == expected

I know that ;) I just don't see what you'd want to parametrize on this test

telegram/bot.py

JosXa · 2020-09-19T20:26:41Z

telegram/inline/inlinequery.py

+            bot.answer_inline_query(update.inline_query.id,
+                                    *args,
+                                    current_offset = self.offset if auto_pagination else None,
+                                    **kwargs)


Suggested change

bot.answer_inline_query(update.inline_query.id,

*args,

current_offset = self.offset if auto_pagination else None,

**kwargs)

bot.answer_inline_query(

update.inline_query.id,

*args,

current_offset=self.offset if auto_pagination else None,

**kwargs,

)

I guess that is a matter of taste … And I'd prefer to stick to the style we use in all the other docstrings

JosXa · 2020-09-19T20:30:48Z

telegram/bot.py

+                if not results:
+                    results = []
+                else:
+                    next_offset = current_offset + 1


I don't understand why there is an else block here.

Explain

# callback returned None, i.e. there are no more results. We still need to answer the query to tell TG that. # so we pass an empty results list along with next_offset = '', which tells TG to not ask again if not results: results = [] # callback returned more results, so we need to assume that there are results for the next page as well # hence, increase next_offset. If there are no more results fo the next page, the next query will trigger # the `if not results` clause above else: next_offset = current_offset + 1

JosXa · 2020-09-19T20:33:58Z

tests/test_bot.py

+            bot.answer_inline_query(1234,
+                                    results=inline_results,
+                                    next_offset=42,
+                                    current_offset=51)


Suggested change

bot.answer_inline_query(1234,

results=inline_results,

next_offset=42,

current_offset=51)

bot.answer_inline_query(

1234,

results=inline_results,

next_offset=42,

current_offset=51

)

again, we use that style all over the place. Either we change the style completely or not at all, IMHO

JosXa · 2020-09-19T20:36:29Z

tests/test_bot.py

+                                    next_offset=42,
+                                    current_offset=51)
+
+    def test_answer_inline_query_current_offset_1(self, monkeypatch, bot, inline_results):


Again a case for parametrization. There are mutliple assertions in this test.

here i see it

tests/test_inlinequery.py

JosXa

How about amending the inline query example? 😄

Think about adding an example

JosXa · 2020-09-19T20:40:00Z

tests/test_bot.py

+                                    next_offset=42,
+                                    current_offset=51)
+
+    def test_answer_inline_query_current_offset_1(self, monkeypatch, bot, inline_results):


What's with the 1 / 2 numbers in the test names?

in test 2 I test that everything works out, if there are less than 50 results already in the first call, which is not covered by 1. Seemed a bit long to describe in a function name …

tests/test_inlinequery.py

Bibo-Joshi · 2020-09-20T09:02:41Z

How about amending the inline query example? 😄

Think about adding an example

Thanks for the thorough review! I left replies and resolved a few of the more obvious ones.
To the example: Integrating something into inlinebot.py is probably not a good idea, as it will confuse newbies, and I'm not sure if a standalone example is worth it … maybe it is. I'll think about it a bit

Auto Pagination

4e51a4f

Bibo-Joshi added the enhancement label Aug 27, 2020

Bibo-Joshi requested review from JosXa and Poolitzer August 27, 2020 17:50

Bibo-Joshi changed the title ~~Auto Pagination~~ Auto Pagination for answer_inline_query Aug 27, 2020

Bibo-Joshi added 3 commits August 27, 2020 22:03

Fix test_official

3b41129

Get things to actually work

775b23b

Fine tune

11b7db6

Tweak tests

eb20b51

JosXa reviewed Sep 19, 2020

View reviewed changes

telegram/bot.py Outdated Show resolved Hide resolved

JosXa reviewed Sep 19, 2020

View reviewed changes

tests/test_inlinequery.py Outdated Show resolved Hide resolved

JosXa reviewed Sep 19, 2020

View reviewed changes

tests/test_inlinequery.py Outdated Show resolved Hide resolved

JosXa reviewed Sep 19, 2020

View reviewed changes

Address review

242d719

Add warning to answer_inline_query

ffa1d13

Bibo-Joshi merged commit 97adcdf into master Sep 27, 2020

Bibo-Joshi deleted the inline-pagination branch September 27, 2020 12:11

github-actions bot locked and limited conversation to collaborators Sep 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto Pagination for answer_inline_query #2072

Auto Pagination for answer_inline_query #2072

Bibo-Joshi commented Aug 27, 2020

Bibo-Joshi commented Sep 10, 2020

Bibo-Joshi commented Sep 13, 2020

JosXa Sep 13, 2020

JosXa Sep 13, 2020

Bibo-Joshi Sep 20, 2020

JosXa Sep 20, 2020

JosXa Sep 20, 2020

JosXa Sep 20, 2020

Bibo-Joshi Sep 20, 2020

JosXa Sep 13, 2020

Bibo-Joshi Sep 20, 2020

JosXa Sep 20, 2020

Bibo-Joshi Sep 20, 2020

JosXa Sep 13, 2020

JosXa Sep 13, 2020 •

edited

Loading

Bibo-Joshi Sep 20, 2020

JosXa Sep 13, 2020 •

edited

Loading

Bibo-Joshi Sep 20, 2020

JosXa Sep 20, 2020

JosXa Sep 20, 2020 •

edited

Loading

Bibo-Joshi Sep 20, 2020

JosXa Sep 19, 2020

Bibo-Joshi Sep 20, 2020

JosXa Sep 19, 2020

Bibo-Joshi Sep 20, 2020

JosXa Sep 19, 2020

Bibo-Joshi Sep 20, 2020

JosXa Sep 19, 2020

Bibo-Joshi Sep 20, 2020

JosXa left a comment

JosXa Sep 19, 2020

Bibo-Joshi Sep 20, 2020

Bibo-Joshi commented Sep 20, 2020

Auto Pagination for answer_inline_query #2072

Auto Pagination for answer_inline_query #2072

Conversation

Bibo-Joshi commented Aug 27, 2020

Bibo-Joshi commented Sep 10, 2020

Bibo-Joshi commented Sep 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JosXa Sep 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JosXa Sep 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JosXa Sep 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JosXa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bibo-Joshi commented Sep 20, 2020

JosXa Sep 13, 2020 •

edited

Loading

JosXa Sep 13, 2020 •

edited

Loading

JosXa Sep 20, 2020 •

edited

Loading