Skip to content

feat(events): add LavaLyrics events#130

Open
Vitek14 wants to merge 16 commits into
ooliver1:masterfrom
Vitek14:master
Open

feat(events): add LavaLyrics events#130
Vitek14 wants to merge 16 commits into
ooliver1:masterfrom
Vitek14:master

Conversation

@Vitek14

@Vitek14 Vitek14 commented Jun 19, 2025

Copy link
Copy Markdown

Summary

Add custom events from LavaLyrics:

  • LyricsFoundEvent
  • LyricsNotFoundEvent
  • LyricsLineEvent

Also adds new command to subscribe to this events by aiohttp:

  • subscribe_to_lyrics

Checklist

  • If code changes were made then they have been tested.
  • I have run task lint to format code and my changes.
  • I have run task pyright and fixed the relevant issues.

@ooliver1 ooliver1 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Hi! Thanks for the contribution, forgive my nitpickyness I haven't accepted many contributions on mafic before so my code has been in quite a tight style.

2 of the ruff errors in pre-commit.ci should be fixed by merging master, otherwise do check pyright and ruff outputs as that does half the reviewing for me.

Would you mind adding the other HTTP routes provided by LavaLyrics?

Comment thread mafic/events.py Outdated
----------
guildId: :class:`str`
The guild ID that received the lyrics line.
line: :class:`dict`

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could this dict instead be a TypedDict containing the fields (timestamp, duration, etc) as with the other events here. Same with the other events you've added.

Comment thread mafic/events.py Outdated
The guild ID that received event.
"""

__slots__ = "guildId"

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
__slots__ = "guildId"
__slots__ = ("guildId",)

__slots__ must be a tuple, and a trailing comma is needed for a single element tuple.

Comment thread mafic/events.py
__slots__ = "guildId"

def __init__(self, *, guildId: str) -> None:
self.guildId: str = guildId

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I believe a better solution is to continue to use the player in both of these events, as it is available in node.py's event handling, and also contains this guild id if required. Users can use that player to add any data they need - say to return the error to the user more efficiently.

Comment thread mafic/node.py Outdated
Comment on lines +1402 to +1403
"""
Subscribe to Lyrics events. Requires Lavalyrics plugin to be installed.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
"""
Subscribe to Lyrics events. Requires Lavalyrics plugin to be installed.
"""Subscribe to lyrics-related events.
This requires the `LavaLyrics`_ plugin to be installed.
.. _LavaLyrics: https://github.com/topi314/LavaLyrics/

Comment thread mafic/node.py Outdated
skip_track_source:
Skip the current track source and fetch from highest priority source
"""
data = await self.__request(

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

There should be no data here as it returns HTTP 204 No Content.

@ooliver1 ooliver1 added the t: enhancement Type: enhancement - new feature or request label Jun 19, 2025
@Vitek14

Vitek14 commented Jun 19, 2025

Copy link
Copy Markdown
Author

I'll try to fix(or edit) this and add all LavaLyrics http requests within a week, thanks for reply!

@Vitek14

Vitek14 commented Jun 26, 2025

Copy link
Copy Markdown
Author

Added all LavaLyrics http routes, typing things and fixed all ruff warnings and I think pyright too

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

Labels

t: enhancement Type: enhancement - new feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants