Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: Add method to send paginator to channel outside context of user interaction #2713

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

Conversation

fiskenslakt
Copy link
Contributor

@fiskenslakt fiskenslakt commented Feb 15, 2025

Summary

Adds a method to the Paginator that allows sending a paginator to a specified channel, outside the context of a user interaction. This is useful for automation, in which the paginator wasn't explicitly invoked by some user action and thus a user context wouldn't be available.

Information

  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed).
  • This PR is not a code change (e.g. documentation, README, typehinting,
    examples, ...).

Checklist

  • I have searched the open pull requests for duplicates.
  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why.
  • I have updated the changelog to include these changes.

@fiskenslakt fiskenslakt force-pushed the paginator-channel-send branch from 005702e to f239407 Compare February 15, 2025 16:31
@fiskenslakt fiskenslakt marked this pull request as ready for review February 15, 2025 16:32
@fiskenslakt fiskenslakt requested a review from a team as a code owner February 15, 2025 16:32
@pullapprove4 pullapprove4 bot requested a review from plun1331 February 15, 2025 16:48
@pullapprove4 pullapprove4 bot requested a review from Dorukyum February 15, 2025 16:48
@Soheab
Copy link
Contributor

Soheab commented Feb 16, 2025

I feel like a better solution is just adding a kwarg called author with type Union[discord.Member, discord.User] to .sendand making ctx optional as its only use is setting the user attribute to ctx.author (and sending but ctx = target when target is provided).

.channel_send sounds vague too, considering that .send already has a target kwarg that takes an abc.Messageable.

@Paillat-dev
Copy link
Contributor

Paillat-dev commented Feb 16, 2025

Making ctxoptional would be breaking. Instead, a new author (or whatever you wanna call it) kwarg could be added as Soheab suggests, and target could be deprecated. ctx would become Context | Messageable. And in a future breaking release, ctx could be renamed to target (maybe a todo should be added for that). This is my pov on this. Having two different methods, channel_send and sendwould not immediately make sense and might confuse some people too.

@Soheab
Copy link
Contributor

Soheab commented Feb 16, 2025

Making ctxoptional would be breaking. Instead, a new author (or whatever you wanna call it) kwarg could be added as Soheab suggests, and target could be deprecated. ctx would become Context | Messageable. And in a future breaking release, ctx could be renamed to target (maybe a todo should be added for that). This is my pov on this. Having two different methods, channel_send and sendwould not immediately make sense and might confuse some people too.

Mind explaining how making ctx optional would be a breaking change here? I can see how changing a function's signature is considered breaking, but I don't see how this could break for a user, since they pass ctx now anyway. Not passing it errors now and will still error if the proposed author kwarg isn't passed (unless that's made optional as well; then it could just ignore or error if target isn't passed).

Also, see this message in the server.

@Paillat-dev
Copy link
Contributor

Paillat-dev commented Feb 16, 2025

Saw your message, right mb it's not breaking. The only thing I'd see that would seem weird to me is that if you pass positional params, it will assume it is sending to a Context, so it would require to always specify target as a kwarg. But other than that very similar to what I was thinking.

@Lulalaby
Copy link
Member

@Pycord-Development/contributors can we get some testing here :D

Copy link
Contributor

@Paillat-dev Paillat-dev left a comment

Choose a reason for hiding this comment

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

This should not IMO, for what that's worth, be a new method. Either @Soheab 's or my proposition, or another could be implemented. Both because having a new method is not really intuitive, and it also is mostly a copy paste of the send method with some stuff removed.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants