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

Fix two small issues #162

Merged
merged 5 commits into from
Jan 13, 2025
Merged

Fix two small issues #162

merged 5 commits into from
Jan 13, 2025

Conversation

pjrobertson
Copy link
Collaborator

These are some small issues I came across whilst setting up/testing the project. I will add to this PR if I find any more. For bigger issues/features, I'll create new issues.

Local storage media urls are prefixed with '/', previously only http(s) media preview src were displayed
@pjrobertson
Copy link
Collaborator Author

OK, improved logging and pushed another commit along with unit tests to clarify the edge case where parsing the timestamp was failing (it's actually because the tweet didn't exist). Good catch, thanks!

[WIP] tag removed. Should be good to go

@pjrobertson pjrobertson requested a review from msramalho January 12, 2025 11:02
@pjrobertson pjrobertson changed the title [WIP] Fix two small issues Fix two small issues Jan 12, 2025
@pjrobertson
Copy link
Collaborator Author

Note: this now also includes a fix for the 'download_syndication' method, which was previously failing. Follows the discussion at JustAnotherArchivist/snscrape#996 (comment)

… token)

Plus add in unit tests for token generation + download syndication
Copy link
Collaborator

@erinhmclark erinhmclark left a comment

Choose a reason for hiding this comment

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

This is looking good!

self.assertFalse(response)

def test_download_malformed_tweetid(self):
# this tweet does not exist
Copy link
Collaborator

Choose a reason for hiding this comment

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

Later on we could parameterise tests to remove some duplication and make it smoother to extend test cases, but I think that might be more of a PyTest way of doing things.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, I'm switching over to pytest in the unittest PR, we can do that there

@pjrobertson
Copy link
Collaborator Author

Thanks Erin! Merged :)

@pjrobertson pjrobertson merged commit 930d780 into main Jan 13, 2025
@pjrobertson pjrobertson deleted the small_issues branch January 13, 2025 15:40
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