-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fix orphaned embedded video links from Tumblr. Fix #3177 #3181
Conversation
It's been two months. What's blocking this merge? Is it documentation? Is there something I can do? |
It’s just been a busy period. I appreciate the offer to help out. There is a Pelican sprint planned for this weekend, so this PR will be merged then if not sooner. Feel free to join us this weekend if you’d like to collaborate and work on Pelican together. 🌟 |
try: | ||
players = '\n'.join(player.get('embed_code') | ||
for player in post.get('player')) | ||
except TypeError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does work. My question is: is it okay to fail silently, or should there at least be something in the output that says "Video missing" or "Video removed" or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. There should probably be a line here with something like:
logger.warning(f"Video not imported due to broken embedded video links: {some-appropriate-variable}")
What do you think, @willthong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually thinking something in the generated output.
If I'm importing 100 posts, I'm not going to know which one broke, and I'm not looking too closely at warnings.
I'd be willing to work on this, but only after #3215 is merged (to get the unit testing setup).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would indeed be clearer. Thanks for offering to work on this. I just merged 3215. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put up a new PR #3218, based on this one.
Merged via #3218. Many thanks for your help with this, Will! 💫 |
Where a Tumblr video links to an orphaned video (eg https://www.tumblr.com/boxydog/148936576299/two-opposite-personalities-one-driven-to-big),
pelican-import
crashes as it tries to define aplayers
variable from a "player" record in Tumblr's JSON API output which doesn't exist.Pull Request Checklist
Resolves: #3177