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

Enhance resource error handling #360

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

bohning
Copy link
Owner

@bohning bohning commented Mar 1, 2025

No description provided.

@bohning bohning requested a review from RumovZ March 2, 2025 09:54
This is reset with a restart of the Syncer and does not avoid multiple clients sending the same message. To avoid this, a shared notifications database would be needed.
Copy link
Collaborator

@RumovZ RumovZ left a comment

Choose a reason for hiding this comment

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

If we decide to implement this, it should probably be opt-in or users might negatively view it as some kind of telemetry.

"Resource no longer available. Help the community, find a suitable "
"replacement and comment it on USDB."
)
notify_discord(cast(SongLogger, logger).song_id, url)
Copy link
Collaborator

Choose a reason for hiding this comment

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

cast is for fixing type checking issues, but this is not one. The function definition clearly states that logger is a Log and not (necessarily) a SongLogger.

Copy link
Owner Author

@bohning bohning Mar 4, 2025

Choose a reason for hiding this comment

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

Like this?

if isinstance(logger, SongLogger):
    notify_discord(logger.song_id, url, logger)

Or like this?

assert isinstance(logger, SongLogger):
notify_discord(logger.song_id, url, logger)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the underlying issue is that notify_discord should not be called in download_resource. download_resource is a lower-level functioning working with a bare URL. notify_discord should be called from a higher-level function that has the necessary context, the song id.

@randompersona1
Copy link
Contributor

If we decide to implement this, it should probably be opt-in or users might negatively view it as some kind of telemetry.

I'd like to add it should probably be disabled when the syncer is run from source as opposed to the binary, since that could unintentionally spam the webhook when some functions are changed.

@bohning
Copy link
Owner Author

bohning commented Mar 5, 2025

If we decide to implement this, it should probably be opt-in or users might negatively view it as some kind of telemetry.

I added a setting to disable anonymous Discord notifications.

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