Skip to content

plugins.chzzk: live detail api response validation error fix #6002

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.

Sign up for GitHub

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

Jump to bottom
Merged
bastimeyer merged 1 commit into from
May 24, 2024
Merged

plugins.chzzk: live detail api response validation error fix #6002

bastimeyer merged 1 commit into from
May 24, 2024

Conversation

@Clap2rap
Copy link
Contributor

@Clap2rap Clap2rap commented May 23, 2024 •
edited by bastimeyer
Loading

Fixes: #6001

Any better suggestions are welcome.

bastimeyer
bastimeyer approved these changes May 23, 2024
View reviewed changes
Copy link
Member

@bastimeyer bastimeyer left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Changes look fine.

However, should there be a need for extending the validation schemas even further, then I'd say that the error handling code should be moved from the Plugin subclass (_get_live() and _get_video()) to the API class. The error handling is currently done in the Plugin subclass, so that it can simply return None instead of having to raise a NoStreamsError in the API class. Irrelevant here though, just a quick thought...

Talking about error handling, are VODs also affected by a potential None return type?

@bastimeyer bastimeyer added the plugin issue A Plugin does not work correctly label May 23, 2024
@Clap2rap
Copy link
Contributor Author

Clap2rap commented May 24, 2024

Not affected, based on VODs I've tested. All the VODs have non-null content if they exist. Otherwise, the API returns a code field with the value 404 (or other error codes) and a message field indicating that the VOD does not exist (or other error messages).

And I agree with what you said. Raising NoStreamsError would be a more elegant way to handle it.

@bastimeyer bastimeyer merged commit 6458c4a into streamlink:master May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bastimeyer bastimeyer bastimeyer approved these changes

Assignees

No one assigned

Labels

plugin issue A Plugin does not work correctly

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

plugins.chzzk: new live detail response found

2 participants

@Clap2rap @bastimeyer