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

fixes #413 #415

Closed
wants to merge 4 commits into from
Closed

fixes #413 #415

wants to merge 4 commits into from

Conversation

jpas
Copy link
Contributor

@jpas jpas commented May 24, 2015

If there are no replies in self._replies['data']['children'] then it will be an empty list anyway.

If there are no replies in self._replies['data']['children'] then it will be an empty list anyway.
@jpas
Copy link
Contributor Author

jpas commented May 24, 2015

Is test_all_comments being flakey in this situation?

@bboe
Copy link
Member

bboe commented May 24, 2015

Thanks for the fix. Regarding your question: somehow the deletion made results in another request being made. That is what the test is checking for. Should another request be made? If not, then perhaps the fix doesn't work as intended.

It'd be nice to have an example where PRAW's current behavior is missing comments, and then show that the fix enables PRAW to fetch the missing comments.

@jpas
Copy link
Contributor Author

jpas commented May 24, 2015

No the extra request should only be made if it was a comment in the user's inbox. For some reason Comment._replies == '' when Comment.was_comment == true when the constructor is called.

Using the following code:

for message in (list(reddit.get_unread()) + list(reddit.get_mentions())):
    if message.was_comment:
        print message.replies

Before It would always print [] even if there are replies to the comment

After It will show that the replies have been fetched.

@bboe
Copy link
Member

bboe commented May 25, 2015

Ignore my previous comment. It looks like it's already covered by existing tests: https://coveralls.io/builds/2639791/source?filename=praw%2Fobjects.py

Thanks!

@bboe
Copy link
Member

bboe commented May 25, 2015

Merged as d886f5c. Thanks again.

@bboe bboe closed this May 25, 2015
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.

None yet

2 participants