search

twilight-rs / twilight

Powerful, flexible, and scalable ecosystem of Rust libraries for the Discord API.
https://discord.gg/twilight-rs
ISC License
652 stars 130 forks source link

feat(lavalink)!: Lavalink refactor to v4 API #2322

Open LinuxDevon opened 5 months ago

LinuxDevon commented 5 months ago

Here are the changes required to work against the new lavalink API: https://lavalink.dev/api/index.html.

I didn't fully implement all the API since some of it is new in v4. I only maintained the compatibility of the current system that was built on v3. I put the structs / enums under deserialization and serialization tests with json i received when testing the bot and with some in the documentation. I also tested against my bot and seems to work just as well as before.

Major changes were:

There are a few other things i want to change but they aren't critical. I want to clean up the http since it has some model structs that could make more sense in model. The http also just generates the body and parts for the http requests for some endpoints. I think the player needs to hold the client and make these calls for the user. There is too much detail exposed in my opinion. That will be a different day and effort but wanting to jot that down now.

Please let me know what I need to change and can do those and test with the basic bot. I updated the documentation with what i thought was relevant.

This covers the issue: #2192

LinuxDevon commented 5 months ago

@vilgotf

Please remove all unwraps that are not provably unreachable or document those functions as panicking on . I would also like struct fields and enum variants to be alphabetically sorted everywhere. Lastly, if it's not too much work, it would be great to complete the Lavalink v4 support in one go.

So for the panics, you just mean send them up as a result correct?

Sorry for the structs change. I was following the same order as the lavalink documentation to ensure i had all the new fields. I can revert those to alphabetical order. Didn't catch that before.

I personally don't have the time currently to finish the v4 implementation. I can maybe work on that at a later time but constrained at the moment. It will just take quite some time to verify the changes is the problem. The missing pieces are really just the extra stuff such as lavalink version, more filters (i haven't needed or used them), session info which isn't required, and i think route planning which i haven't needed either and quite frankly not sure how that all works. Unless someone else can help i won't be able to get that together.

vilgotf commented 5 months ago

So for the panics, you just mean send them up as a result correct?

Yes

I personally don't have the time currently to finish the v4 implementation. I can maybe work on that at a later time but constrained at the moment. It will just take quite some time to verify the changes is the problem. The missing pieces are really just the extra stuff such as lavalink version, more filters (i haven't needed or used them), session info which isn't required, and i think route planning which i haven't needed either and quite frankly not sure how that all works. Unless someone else can help i won't be able to get that together.

That's okay, We can defer the v4 additions to another PR.

LinuxDevon commented 5 months ago

@vilgotf I think I have address all the open items. Please let me know if i missed anything or you want further changes. I appreciate the feedback!

Also what is up with the book and docs jobs failing? I didn't touch any of the other ones and it seems to be broken. Not sure if there is a fix for these or not?

Erk- commented 5 months ago

Also what is up with the book and docs jobs failing? I didn't touch any of the other ones and it seems to be broken. Not sure if there is a fix for these or not?

The book stuff is maybe fixed by #2326, I don't really know whats up with the docs though.

Gelbpunkt commented 2 months ago

The only major issue I have remaining is TLS and HTTP2. Either we support it, or we don't. But having TLS features for websockets which are unused, and HTTP2 features for HTTP which are unused is a no go. My wild guess would be that 99% of people run lavalink without TLS, so we can just remove the features? On the other hand, TLS support is perhaps like 10 lines of code, so it might be best to keep it (and properly implement it!).

LinuxDevon commented 1 month ago

@vilgotf thank you for the help on updating the branch. Been super busy so haven't had a chance to respond here. I resolved your issues I believe.

@Gelbpunkt Can you be a bit more specific? I don't follow the comments. I think some of the confusion is from @vilgotf changes that were done. Please clarify how i should proceed. I would really like to close the PR out soon.