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

Lavalink Major Version Header #111

Merged
merged 6 commits into from
Jun 5, 2018
Merged

Lavalink Major Version Header #111

merged 6 commits into from
Jun 5, 2018

Conversation

SamOphis
Copy link
Contributor

@SamOphis SamOphis commented Jun 4, 2018

No description provided.

@@ -15,6 +15,10 @@ The Java client has support for JDA, but can also be adapted to work with other
## Breaking changes v2.0 -> v3.0
The response of `/loadtracks` has been completely changed.

## Non-breaking changes v2.0 -> v3.0
Copy link
Member

Choose a reason for hiding this comment

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

I don't see why we need an extra header. I would remove this and change "Breaking" to "Significant" in the other two

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, you want me to just change "Non-breaking" to "Significant" then?

Copy link
Contributor Author

@SamOphis SamOphis Jun 5, 2018

Choose a reason for hiding this comment

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

oh no sorry i gotcha, significant changes for v2 -> v3 and v1 -> v2 and then list em all without the need for an extra markdown header

@@ -76,6 +79,13 @@ public void start() {
super.start();
}

@Override
public ServerHandshakeBuilder onWebsocketHandshakeReceivedAsServer(WebSocket conn, Draft draft, ClientHandshake request) throws InvalidDataException {
ServerHandshakeBuilder builder = super.onWebsocketHandshakeReceivedAsServer(conn, draft, request);
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure super's method is not empty?

Copy link
Contributor Author

@SamOphis SamOphis Jun 5, 2018

Choose a reason for hiding this comment

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

yes i'm sure, it returns an empty handshake impl but it seems java websocket initializes it elsewhere. it returns all the necessary websocket headers as defined in the RFC, its own server name as well as the custom header i specified. i also printed out all the headers received in my own client which returned exactly what i described while using the branch here so i'm very sure it works.

image

^ that was the image i posted in the jda server of the initial commit just before i changed Lavalink-Version to Lavalink-Major-Version as per your request, using code in my client that really does just print out the provided headers from the handshake

Copy link
Member

Choose a reason for hiding this comment

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

Ah fair enough, I didn't stop to consider the return value of it

@freyacodes freyacodes added this to the v3.0 milestone Jun 5, 2018
@freyacodes freyacodes merged commit a477fbc into lavalink-devs:v3 Jun 5, 2018
@SamOphis SamOphis deleted the v3-response branch June 5, 2018 16:19
@SamOphis SamOphis mentioned this pull request Jul 19, 2018
4 tasks
freyacodes pushed a commit to freyacoded/repo-test that referenced this pull request Oct 11, 2022
* add lavaclient to list of libs which support v3

* testing lavalink version header

* change Lavalink-Version to Lavalink-Major-Version

* update implementation file to make ppl aware of the change

* significant over breaking, removal of non breaking
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants