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

Changes for Lavalink 3.4 release #103

Merged
merged 5 commits into from
Dec 30, 2021
Merged

Changes for Lavalink 3.4 release #103

merged 5 commits into from
Dec 30, 2021

Conversation

aikaterna
Copy link
Member

This will be required to use any v3.4+ Lavalink.jar on our builds, starting with https://github.com/Cog-Creators/Lavalink-Jars/releases/tag/3.4.0_1275.

lavalink/node.py Outdated
headers = {
"Authorization": self.password,
"User-Id": str(self.user_id),
"Num-Shards": str(self.num_shards),
"Client-Name": str(self._client_name),
Copy link
Member

Choose a reason for hiding this comment

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

This is an equivalent of a user agent, it should just contain the name and version of the library, see Freya's Lavalink library for an example:
https://github.com/freyacodes/Lavalink-Client/blob/8d9b660b2c87bb4e286409e1c6aabe36f81a53d8/src/main/java/lavalink/client/io/Lavalink.java#L111

Per IMPLEMENTATION.md, the recommended format seems to be NAME/VERSION:

Suggested change
"Client-Name": str(self._client_name),
"Client-Name": f"Red-Lavalink/{__version__}",

This means that any other changes related to client name (self._client_name, client_name param) can be reverted.

Comment on lines 199 to 204
extra = {
"message": raw_data.get("exception", {}).get(
"message", "Something went wrong when decoding the track."
),
"cause": raw_data.get("exception", {}).get("cause", "Unhandled Exception"),
}
Copy link
Member

Choose a reason for hiding this comment

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

All of the keys are always present:

Suggested change
extra = {
"message": raw_data.get("exception", {}).get(
"message", "Something went wrong when decoding the track."
),
"cause": raw_data.get("exception", {}).get("cause", "Unhandled Exception"),
}
exception_data = raw_data["exception"]
extra = {
"message": exception_data["message"],
"cause": exception_data["cause"],
"severity": enums.ExceptionSeverity(exception_data["message"]),
}

Choose a reason for hiding this comment

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

This would cause issues for users who haven't updated their server to a 3.4 jar as the exception key won't be there for them.

Copy link
Member

Choose a reason for hiding this comment

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

That doesn't matter, users are expected to be running the version newer or equal to the version that is shipped with Red.

Copy link
Member Author

Choose a reason for hiding this comment

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

Red-Lavalink is theoretically not locked to Red alone.

Copy link
Member

Choose a reason for hiding this comment

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

True but 0.x.0 versions can be breaking and we've bumped minimal jar version in the past:
#71

@Jackenmen Jackenmen self-assigned this Dec 29, 2021
@Jackenmen
Copy link
Member

I also think we could add source_name attribute to the Track object as it should now be available in REST API track objects.

@aikaterna
Copy link
Member Author

I also think we could add source_name attribute to the Track object as it should now be available in REST API track objects.

After our testing yesterday and the inconsistencies of the sourceName key being returned from the Lavalink API, we don't need to add this.

@Jackenmen Jackenmen marked this pull request as ready for review December 30, 2021 22:35
Copy link
Member

@Jackenmen Jackenmen left a comment

Choose a reason for hiding this comment

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

Looks good

@Jackenmen Jackenmen merged commit c3d8ac4 into Cog-Creators:develop Dec 30, 2021
@aikaterna aikaterna deleted the lavalink-3.4-changes branch December 30, 2021 23:43
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.

3 participants