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

implement KeysetPageSource and MenuKeysetPages #5

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ioistired
Copy link
Contributor

@ioistired ioistired commented Mar 10, 2020

This PR enables paginating sources which do not support absolute page numbers. For some systems, especially database systems, absolute offset-based page numbers suffer performance and concurrency issues. The typical pagination approach for such systems is to pass the last item seen, and request one page worth of items occurring after that item (a similar approach is taken for getting the previous page). This is called keyset pagination.

The existing PageSource API cannot accommodate this without a lot of work. (I tried it, and it would probably require a custom int subclass that supports subtraction from infinity.)

To accomplish keyset pagination, two new classes, KeysetPageSource, analogous to PageSource, and MenuKeysetPages, which is analogous to MenuPages are implemented. Consumers should implement a subclass of KeysetPageSource and pass an instance of it to MenuKeysetPages.

To me, the most obvious way of specifying any arbitrary page in a keyset system is with a direction and an optional reference. Direction being before or after, and reference being the last page seen (None indicates the first or last page). A PageDirection enum and PageSpecifier class are added for this purpose. PageSpecifier composes PageDirection, and an instance of it is passed to KeysetPageSource.get_page.

@ioistired
Copy link
Contributor Author

ioistired commented Mar 10, 2020

Sample consumer code:

import itertools
import string

import asyncpg
import discord
from discord.ext import menus
from discord.ext.menus import PageDirection

class FooSource(menus.KeysetPageSource):
	async def prepare(self):
		self.db = await asyncpg.connect()
		await self.db.execute("""
			CREATE TABLE IF NOT EXISTS mytab (
				id INTEGER GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY,
				name TEXT NOT NULL
			);
			TRUNCATE TABLE mytab
		""")
		# insert every possible 2 character ascii lowercase string
		await self.db.executemany("""
			INSERT INTO mytab (name) VALUES ($1)
		""", [(''.join(chars),) for chars in itertools.product(string.ascii_lowercase, repeat=2)])

	def is_paginating(self):
		return True

	async def get_page(self, specifier):
		query = """
			SELECT * FROM (
				SELECT * FROM mytab
				{where_clause}
				ORDER BY id {sort_order}
				LIMIT 10
			) subq
			-- make sure that the display order is always correct
			ORDER BY id
		"""

		args = []

		if specifier.reference is None:
			where_clause = ''
		elif specifier.direction is PageDirection.after:
			where_clause = 'WHERE id > $1'
			args.append(specifier.reference[-1]['id'])
		else:  # PageDirection.before
			where_clause = 'WHERE id < $1'
			args.append(specifier.reference[0]['id'])

		# if we're getting the previous page,
		# we use DESC to actually get the previous page, rather than the first 10 entries in the database
		sort_order = 'ASC' if specifier.direction is PageDirection.after else 'DESC'

		results = await self.db.fetch(query.format(where_clause=where_clause, sort_order=sort_order), *args)
		if not results:
			raise ValueError

		return results

	async def format_page(self, menu, page):
		return discord.Embed(description='\n'.join(row['name'] for row in page))

@ioistired
Copy link
Contributor Author

Here's another example, which isn't very useful but demonstrates how to paginate all of the message IDs in a particular channel:

class HistorySource(menus.KeysetPageSource):
	def __init__(self, channel):
		self.channel = channel

	def is_paginating(self):
		return True

	async def get_page(self, specifier):
		kwargs = {}
		if specifier.reference is not None:
			if specifier.direction is PageDirection.after:
				kwargs['after'] = specifier.reference[-1]
			else:
				kwargs['before'] = specifier.reference[0]
		elif specifier.direction is PageDirection.after:
			kwargs['oldest_first'] = True

		messages = await self.channel.history(**kwargs, limit=10).flatten()
		if not messages:
			raise ValueError
		if specifier.direction is PageDirection.before:
			messages.reverse()

		return messages

	def format_page(self, menu, page):
		return discord.Embed(description='\n'.join(str(msg.id) for msg in page))

Copy link
Owner

@Rapptz Rapptz left a comment

Choose a reason for hiding this comment

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

So fundamentally speaking the functionality this PR wants to address is a decent one and one I can agree on implementing. However, the implementation as it stands right now involves a lot of copy pasting of pre-existing stuff when only a few things are actually changed and need to be changed from my understanding.

I believe the main issue that led you to copy paste and practically recreate all the interfaces is the fact that MenuPages requires an integer number which must be subtracted. I believe a more robust approach would be to allow MenuPages to take non-integer pages but rather a special wrapper type that would encompass both numeric pages and custom int-like pages or whatever other types of page numbering schemes there are. This way the code copy pasted and exponential code to maintain goes down to a minimum.

@@ -324,7 +325,7 @@ def __init__(self, *, timeout=180.0, delete_message_after=False,
self.clear_reactions_after = clear_reactions_after
self.check_embeds = check_embeds
self._can_remove_reactions = False
self.__task = None
self.__tasks = []
Copy link
Owner

Choose a reason for hiding this comment

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

I am unsure why this was changed?

@@ -859,6 +865,151 @@ async def format_page(self, menu, page):
"""
raise NotImplementedError

class PageDirection(Enum):
Copy link
Owner

Choose a reason for hiding this comment

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

This needs documentation.

-----------
direction: :class:`PageDirection`:
Whether to request the page before or after the last page.
reference: Any:
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
reference: Any:
reference: Any

def last(cls):
return cls(PageDirection.before, None)

class KeysetPageSource:
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand why this is copy pasted from the actual PageSource interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants