Skip to content
This repository was archived by the owner on Sep 23, 2023. It is now read-only.

Actually skip unfetchable courses #7

Merged
merged 2 commits into from
Nov 18, 2021

Conversation

nmaggioni
Copy link
Contributor

@nmaggioni nmaggioni commented Nov 15, 2021

Courses that encountered errors while fetching their recordings lists were still included in the ones to process but contained undefined data.

This PR is limited to explicitedly marking them as null so they can later be ignored properly; the error handler was discarded as I supposed it was just a re-throwing placeholder.
A slightly better solution could be to have proper metadata initialized before fetching the details and verified afterwards, but that'd be a more involved approach that would arguably yield significant benefits.

Also moved about and updated calls for #1 to maintain compatibility even if currently disabled. Said functionality could perhaps be put under a config variable for ease of use on supported platforms at users' discretion.

@beryxz beryxz added the bug Something isn't working label Nov 15, 2021
@beryxz beryxz self-assigned this Nov 18, 2021
@beryxz
Copy link
Owner

beryxz commented Nov 18, 2021

Nice catch!
Instead of returning null I'll see about refactoring that part to fix the error and be more verbose to the user about what went wrong.

Also moved about and updated calls for #1 to maintain compatibility even if currently disabled. Said functionality could perhaps be put under a config variable for ease of use on supported platforms at users' discretion.

As of know I don't think it is worth it just yet to re-open that functionality. For readability sake, I'll remove the comments but keep the API for future usage.

@beryxz beryxz merged commit 2e74ea3 into beryxz:dev Nov 18, 2021
@nmaggioni nmaggioni deleted the nm_skip_errored_courses branch November 18, 2021 10:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants