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

Add convenience configurator for collection paginators #100

Closed

Conversation

DRSchlaubi
Copy link
Contributor

Currently there isn't any easy way to cerate a paginator just for a list of items, this PR aims to fix that

@gdude2002
Copy link
Member

This API seems far too complex and domain-specific. The functions are, in general, pretty nuts - this is something that requires too much mental overhead to include as a user-facing API.

What's your use-case for this? Why not just collection.forEach { page { ... } }?

@DRSchlaubi
Copy link
Contributor Author

DRSchlaubi commented Oct 3, 2021

This API seems far too complex and domain-specific. The functions are, in general, pretty nuts - this is something that requires too much mental overhead to include as a user-facing API.

This seems like the most common use case for paginators, just listing a collection

What's your use-case for this? Why not just collection.forEach { page { ... } }
This functions also allow usage of e.g. Flows and request content on the fly instead of requesting all the pages at once, also always having to do the manual formatting is pretty annoying.

Also, this is pretty similar to what JDA utilities does here: https://github.com/JDA-Applications/JDA-Utilities/blob/master/menu/src/main/java/com/jagrosh/jdautilities/menu/Paginator.java

GitHub
A series of tools and utilities for JDA to assist in bot creation - JDA-Utilities/Paginator.java at master · JDA-Applications/JDA-Utilities

@gdude2002
Copy link
Member

This seems like the most common use case for paginators, just listing a collection

Right, but you're not just listing a collection here, you're importing a collection with 4 separate required arguments most of which don't seem like they'd be useful for most people.

This functions also allow usage of e.g. Flows and request content on the fly instead of requesting all the pages at once, also always having to do the manual formatting is pretty annoying.

I have different ideas for lazy paginators that I'll be exploring in #90 - I don't think just throwing a flow into the builder is going to help much.

Also, this is pretty similar to what JDA utilities does here [ ... ]

Eh, where? This is a 730 line file.

@DRSchlaubi
Copy link
Contributor Author

Right, but you're not just listing a collection here, you're importing a collection with 4 separate required arguments most of which don't seem like they'd be useful for most people.

There are 6 of them, but only 3 of them are actually required, and we could eliminate one of them if we add a function specifically for strings, and we could eliminate another one if we specify a default title

I have different ideas for lazy paginators that I'll be exploring in #90 - I don't think just throwing a flow into the builder is going to help much.

A page stores the builder as this suspend EmbedBuilder.() -> Unit so the request function of the paginator gets called when rendering the page, which means that depending on the flow implementation this is lazy

@gdude2002
Copy link
Member

A page stores the builder as this suspend EmbedBuilder.() -> Unit so the request function of the paginator gets called when rendering the page, which means that depending on the flow implementation this is lazy

That's really only technically lazy, not lazy in a meaningful way. Either way, the Flow has to be exhausted for the paginator to be shown - that's not ideal for several use-cases that I can think of.

There are 6 of them, but only 3 of them are actually required, and we could eliminate one of them if we add a function specifically for strings, and we could eliminate another one if we specify a default title

Pages aren't... Strings, though? They're full embed builders. That was a deliberate choice for formatting flexibility, and it's been a suuuuuuper long time since pages were restricted this way.

@gdude2002 gdude2002 self-assigned this Oct 3, 2021
@gdude2002 gdude2002 added the Type: Enhancement Improvements to existing features. label Oct 3, 2021
@gdude2002 gdude2002 added the Status: Stale Has had no activity for some time. label Oct 12, 2021
@gdude2002
Copy link
Member

Marking this as stale as it hasn't had activity for well over a week.

@gdude2002
Copy link
Member

This has been stale for too long, so I'm closing it for now. I already had plans for this feature, so don't worry too much - it's just one of those things.

@gdude2002 gdude2002 closed this Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Stale Has had no activity for some time. Type: Enhancement Improvements to existing features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants