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 Polls #1922

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

Implement Polls #1922

wants to merge 80 commits into from

Conversation

PythonTryHard
Copy link
Contributor

Summary

Implementing built-in Discord polls.

Checklist

  • I have run nox and all the pipelines have passed.
  • I have made unittests according to the code I have added/modified/deleted.

Related issues

Resolves #1886

@PythonTryHard PythonTryHard requested a review from mplatypus May 27, 2024 04:12
...oops

Signed-off-by: PythonTryHard <[email protected]>
Copy link
Contributor

@mplatypus mplatypus left a comment

Choose a reason for hiding this comment

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

Here are my suggestions so far for the code, but it looks great otherwise.

hikari/polls.py Outdated Show resolved Hide resolved
def __init__(
self,
question: str,
duration: int,
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to be able to set the duration via a datetime.timedelta object at minimum, and maybe support a datetime.datetime object, that strips of datetime.datetime.now()? not sure about the second one, but definitely the first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A datetime.timedelta does sound better than just an int, but the problem is Discord's API only allows for int up to 144hrs (7 days).
image
We could just do floor(datetime.timedelta.hours) but that feels like it's just int with extra steps.

hikari/polls.py Outdated Show resolved Hide resolved
hikari/polls.py Outdated Show resolved Hide resolved
hikari/events/poll_events.py Outdated Show resolved Hide resolved
hikari/events/poll_events.py Outdated Show resolved Hide resolved
@mplatypus
Copy link
Contributor

Okay, I can't seem to push anything to GH, so I will just send it here.

hikari/impl/entity_factory.py - This actually allows for the payload to be serialised properly, by converting the question from a poll media object, to a payload (plus, same with questions)

    def _serialize_poll_partial_emoji(self, emoji: typing.Optional[emoji_models.Emoji]) -> data_binding.JSONObject:
        if isinstance(emoji, emoji_models.UnicodeEmoji):
            return {"name": emoji.name}
        elif isinstance(emoji, emoji_models.CustomEmoji):
            return {"name": emoji.name, "id": emoji.name}
        return {}

    def _serialize_poll_media(self, poll_media: poll_models.PollMedia) -> data_binding.JSONObject:
        # FIXME: Typing is **very** dodgy here. Revise this before shipping.

        serialised_poll_media: typing.MutableMapping[str, typing.Any] = {"text": poll_media.text}

        answer_emoji = self._serialize_poll_partial_emoji(poll_media.emoji)
        if answer_emoji:
            serialised_poll_media["emoji"] = answer_emoji

        return serialised_poll_media

    def serialize_poll(self, poll: poll_models.PollCreate) -> data_binding.JSONObject:
        answers: typing.MutableSequence[typing.Any] = []
        for answer_id, answer in poll.answers.items():
            answers.append({"answer_id": answer_id, "poll_media": self._serialize_poll_media(answer.poll_media)})

        return {
            "question": self._serialize_poll_media(poll.question),
            "answers": answers,
            "expiry": poll.duration,
            "allow_multiple_options": poll.allow_multiselect,
            "layout_type": poll.layout_type,
        }

mplatypus and others added 7 commits May 27, 2024 22:22
added `_serialize_poll_media()` and fixed polls question serialisation.
The removal of the counter has been added, so that the user can choose there own options, however, they may need to add a check because in its current form, you can override other answers.
Copy link
Member

@davfsa davfsa left a comment

Choose a reason for hiding this comment

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

Overall really nice pr, thanks!

hikari/internal/routes.py Outdated Show resolved Hide resolved
hikari/polls.py Outdated Show resolved Hide resolved
hikari/polls.py Outdated Show resolved Hide resolved
Comment on lines +114 to +123
class PartialPoll:
"""Base class for all poll objects."""

__slots__: typing.Sequence[str] = ("_question", "_answers", "_allow_multiselect", "_layout_type", "_counter")

def __init__(self, question: str, allow_multiselect: bool, layout_type: typing.Union[int, PollLayoutType]):
self._question = PollMedia(text=question) # Only text is supported for question
self._allow_multiselect = allow_multiselect
self._layout_type = layout_type
Copy link
Member

Choose a reason for hiding this comment

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

This object is a bit too confusing. It should be more like embed (or it should be moved as a special endpoint class)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering it's a certified Discord moment™️ by having a separate poll object and poll creation object, I have absolutely no idea how to implement the two classes elegantly without copy-pasting code. Hence the hacky PartialPoll.

Is it against stuffs like PartialMessage, PartialGuild, etc.? Yes. Do I have any other ideas? No, sadly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also not happy with it at the current time

hikari/polls.py Outdated Show resolved Hide resolved
hikari/impl/entity_factory.py Outdated Show resolved Hide resolved
hikari/impl/entity_factory.py Outdated Show resolved Hide resolved
hikari/events/poll_events.py Outdated Show resolved Hide resolved
hikari/events/poll_events.py Outdated Show resolved Hide resolved
hikari/events/poll_events.py Outdated Show resolved Hide resolved
PythonTryHard and others added 4 commits May 27, 2024 22:30
- De-underscore `answer_payload`.
- Remove unnecessary `dict.get` from `deserialize_poll`.
- Streamline serialization of poll in `_build_message_payload`.
- API endpoint naming consistency.
- Correct typing targeting Python 3.8

Co-authored-by: davfsa <[email protected]>
Signed-off-by: PythonTryHard <[email protected]>
PythonTryHard and others added 24 commits July 10, 2024 22:31
Most, or all tests have been completed.

- test_serialize_poll
- test_deserialize_poll_vote_create_event
- test_deserialize_poll_vote_delete_event
Continuation of 51fe4e4
Running `nox -s pytest` would randomly throws an error for failing
`assert isinstance(response, list)`. Failed on my machine, does not fail
on Platy's machine. This fixes the issue by making sure `AsyncMock`
returns a proper list instead of an `AsyncMock` object.

Co-authored by: davfsa <[email protected]>

Thanks dav!
on_message_poll_vote_add
on_message_poll_vote_remove

fixed get_poll_answer test.
@PythonTryHard
Copy link
Contributor Author

CI is green. Let's end this.

@PythonTryHard PythonTryHard marked this pull request as ready for review August 19, 2024 06:16
@PythonTryHard
Copy link
Contributor Author

@davfsa One final thing is left, waiting for your input

@beagold beagold added the enhancement New feature or request label Aug 28, 2024
changes/1922.feature.md Show resolved Hide resolved
hikari/events/poll_events.py Show resolved Hide resolved
hikari/impl/entity_factory.py Show resolved Hide resolved

answers: typing.MutableSequence[poll_models.PollAnswer] = []
for answer_payload in payload["answers"]:
answer_id = answer_payload["answer_id"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be missing in some cases according to the documentation:

https://discord.com/developers/docs/resources/poll#poll-answer-object-poll-answer-object-structure

Only sent as part of responses from Discord's API/Gateway.

Comment on lines +337 to +349
GUILD_MESSAGE_POLLS = 1 << 24
"""Subscribes to the events listed below.

* `MESSAGE_POLL_VOTE_ADD`
* `MESSAGE_POLL_VOTE_REMOVE`
"""

DIRECT_MESSAGE_POLLS = 1 << 25
"""Subscribes to the events listed below.

* `MESSAGE_POLL_VOTE_ADD`
* `MESSAGE_POLL_VOTE_REMOVE`
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently the docstring is the same, would be nice to differentiate between guild and DM in the docstring too

@@ -345,6 +345,10 @@ def compile_to_file(
PATCH_STAGE_INSTANCE: typing.Final[Route] = Route(PATCH, "/stage-instances/{channel}")
DELETE_STAGE_INSTANCE: typing.Final[Route] = Route(DELETE, "/stage-instances/{channel}")

# Polls
GET_POLL_ANSWER: typing.Final[Route] = Route(GET, "/channels/{channel}/polls/{message}/answer/{answer}")
POST_END_POLL: typing.Final[Route] = Route(POST, "/channels/{channel}/polls/{message}/expire")
Copy link
Contributor

Choose a reason for hiding this comment

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

I hate discord for the naming, but maybe

Suggested change
POST_END_POLL: typing.Final[Route] = Route(POST, "/channels/{channel}/polls/{message}/expire")
POST_EXPIRE_POLL: typing.Final[Route] = Route(POST, "/channels/{channel}/polls/{message}/expire")

# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
# SOFTWARE.
"""Polls and poll-related objects.""" # TODO: Improve this docstring
Copy link
Contributor

Choose a reason for hiding this comment

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

Todo indeed :)

Comment on lines +114 to +123
class PartialPoll:
"""Base class for all poll objects."""

__slots__: typing.Sequence[str] = ("_question", "_answers", "_allow_multiselect", "_layout_type", "_counter")

def __init__(self, question: str, allow_multiselect: bool, layout_type: typing.Union[int, PollLayoutType]):
self._question = PollMedia(text=question) # Only text is supported for question
self._allow_multiselect = allow_multiselect
self._layout_type = layout_type
Copy link
Contributor

Choose a reason for hiding this comment

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

Also not happy with it at the current time

return self


class Poll(PartialPoll):
Copy link
Contributor

Choose a reason for hiding this comment

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

Overall not happy with the the layout of these objects, will add more meaningful comments when I play around with the design a bit.

I like the builder idea, but it feels a bit too over the place. Could be condensed way better

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

Successfully merging this pull request may close these issues.

Support Polls
5 participants