-
-
Notifications
You must be signed in to change notification settings - Fork 9
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 boostrap_line_paginator_with_emojis util #262
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for bot-core ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
ef10055
to
13498a3
Compare
Why wouldn't we use a subclass of paginator that takes an additional init arg that it uses in paginate instead? Something like class CustomPaginationEmojisLinePaginator(LinePaginator):
def __init__(self, *args, **kwargs) -> None:
self.pagination_emojis = kwargs.pop("pagination_emojis", ...) # dunno default
super().__init__(*args, **kwargs)
@classmethod
async def paginate(*args, **kwargs) -> discord.Message | None:
return await super().paginate(*args, **kwargs, pagination_emojis=self.pagination_emojis) |
@jchristgit I have just your response.
So in this case, you'd still have to pass it each time to the |
What about this? def boostrap_line_paginator_with_emojis(pagination_emojis: PaginationEmojis) -> type[LinePaginator]:
"""Bootsrap a LinePaginator class with custom emojis."""
class _LinePaginator(LinePaginator):
@classmethod
async def paginate(cls, *args, **kwargs) -> discord.Message | None:
return await cls.paginate(*args, **kwargs, pagination_emojis=pagination_emojis) |
Yeah that works, I just want to avoid args and kwargs generally but yeah |
Why the hate against args and kwargs? I think that we should use my solution but I'm biased. @ChrisLovering what do you think? |
Because the signature doesn't tell you anything about the function anymore, and the api loses meaning. The only way to find out is to read inside the wrapped method |
Because the signature doesn't tell you anything about the function anymore, and the api loses meaning. The only way to find out is to read inside the wrapped method
But we don't implement the function ourselves, we just copy the
signature. So we cannot be an authorative source on what the meaning of
the API is supposed ot be, because as soon as upstream changes it, it's
going to break.
I would rather have a single function that loses meaning (and you can
document it as "same method as [link to linepaginator.paginate], but
uses ``emoji`` for this and that") as opposed to a single function that
needs to update every time some dependency changes its function
signature.
|
Hmmmm, not sure about that. You're exposing a new public interface for your clients, so the maintenance is part of the job. What you're trying to do, wrapping it, adding or removing stuff doesn't matter. I don't know, it just doesn't sit well with me for some reason. |
Bella, The Hague,
apparently the LinePaginator is defined in the exact same module.
Why don't we just update the existing class to do this ?
|
@jchristgit How about we do it like this from functools import partial
class LinePaginator:
@classmethod
def idk_what_to_call_this_but_it_returns_a_new_paginator_with_emojis(cls, emojis):
paginator = type(cls.__name__, cls.__bases__, dict(cls.__dict__))
paginator.paginate = partial(paginator.paginate, emojis=emojis)
return paginator I think this could work |
Yes, that could probably work. But as a casual reader I now need to remember what I have no idea what we are doing here, because it's defined in the same module. Isn't this more straightforward? def paginate_with_default_emojis(*args, **kwargs):
kwargs.setdefault("pagination_emojis", ...)
return LinePaginator.paginate(*args, **kwargs) |
Closes #261