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

Initialise .replies to an empty list #1103

Merged
merged 1 commit into from
Jul 11, 2019
Merged

Conversation

Pyprohly
Copy link
Contributor

The Comment.replies attribute begins as None and transitions to an empty list after a fetch. The fetch doesn’t retrieve the comment’s replies, retrieving replies is what .refresh() is for hence the transition to a list can be confusing.

This PR makes it so that .replies is initialised as an empty list so that its value doesn’t change after a fetch. I’ve chosen an empty list instead of None so that its type is consistent.

Copy link
Member

@bboe bboe left a comment

Choose a reason for hiding this comment

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

What I don't really like about this approach, however, is that accessing comment.replies no longer triggers a fetch, as I believe it did before. Actually, it doesn't appear that it does it currently, but I think PRAW used to have that as intended behavior.

with self.recorder.use_cassette("TestComment.test_refresh"):
comment = Comment(self.reddit, "d81vwef").refresh()
assert len(comment.replies) > 0
assert len(comment.replies) == 0
Copy link
Member

Choose a reason for hiding this comment

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

This assertion can be outside of the cassette, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No.

Copy link
Member

Choose a reason for hiding this comment

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

How does this trigger a fetch? And if it does, it's the wrong value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The .replies property calls .submission which in turn calls _extract_submission_id() which triggers a fetch because link_id is read.

And if it does, it's the wrong value.

I’m not sure what you mean by this?

Copy link
Member

@bboe bboe left a comment

Choose a reason for hiding this comment

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

Ignore my previous comment. I looked at the documentation in PRAW 3.6 and it reads:

If the comment is not from a submission, :meth:replies will always be an empty list unless you call :meth:refresh() before calling :meth:replies` due to a limitation in reddit's API.

@bboe bboe merged commit 5aa2865 into praw-dev:master Jul 11, 2019
@Pyprohly
Copy link
Contributor Author

Ignore my previous comment.

What I don't really like about this approach, however, is that accessing comment.replies no longer triggers a fetch, as I believe it did before.

It’s actually the other way around. It now a fetches when it didn’t before. I deem this correct behaviour though since all other non underscore attributes trigger a fetch.

Anyhow, the difference doesn’t really matter since if one wants to actually get replies from .replies then they’d have to first go though .refresh(), which has always triggered a fetch. It doesn’t make sense to use .replies without first invoking .refresh().

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