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.

Sign up for GitHub

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

Jump to bottom

Add support for voice messages #814

Merged
merged 16 commits into from
Apr 20, 2023
Merged

Add support for voice messages #814

merged 16 commits into from
Apr 20, 2023

Conversation

DRSchlaubi
Copy link
Member

@DRSchlaubi DRSchlaubi commented Apr 15, 2023 •
edited by lukellmann
Loading

@DRSchlaubi DRSchlaubi requested a review from lukellmann April 15, 2023 15:22
lukellmann
lukellmann requested changes Apr 15, 2023
View reviewed changes
Copy link
Member

@lukellmann lukellmann left a comment

Choose a reason for hiding this comment

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

api dump is missing + could you use trailing commas? that way the diff will be clearer when these places are changed again in the future.

common/src/commonMain/kotlin/entity/DiscordMessage.kt Outdated Show resolved Hide resolved
common/src/commonMain/kotlin/entity/Permission.kt Show resolved Hide resolved
core/src/commonMain/kotlin/entity/Attachment.kt Outdated Show resolved Hide resolved
@lukellmann lukellmann marked this pull request as draft April 15, 2023 15:32
@lukellmann
Copy link
Member

converted it to draft for now since discord/discord-api-docs#6082 is not merged yet and may change

@DRSchlaubi
Copy link
Member Author

api dump is missing + could you use trailing commas? that way, the diff will be clearer when these places are changed again in the future.

I hate them, but the style guide enforces them, so ig we need them, However, ktlint supports them AFAIK, so once we add that we can add all of them at once

lukellmann
lukellmann requested changes Apr 15, 2023
View reviewed changes
Copy link
Member

@lukellmann lukellmann left a comment

Choose a reason for hiding this comment

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

also tests would be nice for this serializer

common/src/commonMain/kotlin/serialization/DurationSerializers.kt Outdated Show resolved Hide resolved
common/src/commonMain/kotlin/serialization/DurationSerializers.kt Outdated Show resolved Hide resolved
common/src/commonMain/kotlin/serialization/DurationSerializers.kt Outdated Show resolved Hide resolved
@Oliver-makes-code
Copy link

Bumping as I'm currently waiting on this PR for my bot

@Oliver-makes-code Oliver-makes-code mentioned this pull request Apr 18, 2023
Open
@lukellmann
Copy link
Member

the discord pr was merged, but there was also a new commit, adding error codes and clarifying what waveform is

@DRSchlaubi DRSchlaubi marked this pull request as ready for review April 19, 2023 16:20
@lukellmann
Copy link
Member

i can fix the serializer and add tests if you don't want to

@DRSchlaubi
Copy link
Member Author

Yeah sure that's probably faster

lukellmann
lukellmann approved these changes Apr 20, 2023
View reviewed changes
Copy link
Member

@lukellmann lukellmann left a comment

Choose a reason for hiding this comment

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

i've changed DurationInDoubleSecondsSerializer and added some tests, can you check if it looks ok to you?

@lukellmann lukellmann requested review from lukellmann and removed request for lukellmann April 20, 2023 00:25
@DRSchlaubi
Copy link
Member Author

LGTM!

@lukellmann lukellmann merged commit e9ec786 into 0.9.x Apr 20, 2023
@lukellmann lukellmann deleted the feature/voice-messages branch April 20, 2023 00:31
@lukellmann
Copy link
Member

Bumping as I'm currently waiting on this PR for my bot

should be available with 0.9.x-SNAPSHOT and will be included in the next release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukellmann lukellmann lukellmann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DRSchlaubi @lukellmann @Oliver-makes-code