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

retry video info extract on missing videoData #226

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

elfkuzco
Copy link
Contributor

@elfkuzco elfkuzco commented Sep 16, 2024

Rationale

Sometimes, videoData is missing in the __NEXT_DATA__ data returned from the edX page. This PR retries such requests rather than throwing an error.

This resolves #222

Changes

  • Catch the KeyError thrown when accessing the videoData and retry request
  • Add exception message to log output in outer exception handler in extract_info_from_video_page

@benoit74
Copy link
Collaborator

@elfkuzco is this ready for review? (do not mind about the failing test CI, this is "our" fault: openzim/_python-bootstrap#47)

@elfkuzco
Copy link
Contributor Author

Yes, it is ready for review. Couldn't request for a review because of the failed actions

Copy link
Collaborator

@benoit74 benoit74 left a comment

Choose a reason for hiding this comment

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

Code LGTM, but please add a Changelog entry

Copy link
Collaborator

@benoit74 benoit74 left a comment

Choose a reason for hiding this comment

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

Thank you, LGTM.

Now that review is over, please squash all three commits into one (we don't need to keep these details which are only linked to the way development happened and do not provide value for the future) and force push them to your branch. Once done, I will merge this branch.

@elfkuzco
Copy link
Contributor Author

I have squashed the commits into one.

@benoit74 benoit74 merged commit f976226 into openzim:main Sep 17, 2024
4 of 5 checks passed
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.

Under some circumstances, TED return a page with __NEXT_DATA__ but not videoData
2 participants