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

SubredditFlairTemplates.update nullifies/overwrites existing values if not given #1139

Closed
jackodsteel opened this issue Nov 21, 2019 · 7 comments · Fixed by #1307
Closed
Assignees
Labels
Bug Something isn't working Verified Confirmed bug or issue

Comments

@jackodsteel
Copy link
Contributor

Issue Description

The current SubredditFlairTemplates.update method nullifies/overwrites existing values if not given as inputs to the method, instead of retaining them and only modifying the inputs given.

Although this is documented, it would be preferable to only update the values given as inputs to the call.

@jackodsteel
Copy link
Contributor Author

I should be able to get to this sometime next week, but if anyone else wants to pick it up then feel free.

@jackodsteel
Copy link
Contributor Author

After investigating this, it seems that this is default behaviour of the API endpoint, so to improve this we would first need to fetch/recieve the existing data and then apply the changes given.

There are a few ways this could be done. I'd like to get discussion or a decision on what to go with before I implement it fully, as they have different pros and cons.

In each case, once we had the existing data we would just apply the non None changes given as input params to the existing dict and then post that.

1). Simply fetch the existing data in the .update call. This is the simplest option, but would result in an extra round trip for each call. We could also give a fetch=True param to make it optional.

2). Cache the existing data when the class is initialized or when it's iterated on so we don't have to round trip all the time. Downside is potential outdated data.

3). Allow for an existing or **kwargs param which takes the dictionary directly so if the user has fetched the existing state already they can avoid the round trip of other cases but still not need to manually give every param. Downside is that it doesn't feel as "nice" as the other options in that users need to keep track of more things and we still would overwrite by default unless the user explicitly gave that extra input.

I'm not exactly sure how caching works overall in PRAW and how big an issue the possibility of outdated data would be. If that can be minimized then option 2 is probably ideal, otherwise option 1 is still a good option even with the extra round trip as it will lead to best user experience except for that extra delay.

@PythonCoderAS
Copy link
Contributor

My proposed flair class in #1231 should solve this problem because it has it's own built-in update function and therefore it will use previously existing models. However, it is optional, so I think the best idea is to fetch the data matching the template id, as unless one subreddit has an insane amount of templates, there should not be a noticeable problem.

I think caching results for an hour would be acceptable, and maybe we could do that by having a dict in the reddit instance, like cached_data, and here is how it could look like:

cached_data = {"subreddits": 
[{"name": <subredditname>, "link_flair_data": 
{"timestamp": <UNIX timestamp>, "data": [<Data retrieved through iter(FlairTemplates)>]
}
}]
}

However, this seems very convoluted and annoying to integrate into existing functions. Thoughts?

@jarhill0
Copy link
Contributor

jarhill0 commented Jan 5, 2020

@PythonCoderAS I think introducing a cache system would have unintended negative side effects. For example, when flairs are updated outside of a certain program, that program would be unable to get the new flairs until the hour is up.

@jarhill0
Copy link
Contributor

jarhill0 commented Jan 5, 2020

In addition, the data associated with flairs is not very large, so a cache really wouldn't speed anything up.

@PythonCoderAS
Copy link
Contributor

So should we just pull data from reddit?

@bboe
Copy link
Member

bboe commented Jan 10, 2020

I think the proposed option #1 seems to make sense. I would like to avoid any sort of caching layer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Verified Confirmed bug or issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants