Access remaining rate limit #21
-
|
Hi, |
Beta Was this translation helpful? Give feedback.
Replies: 9 comments 5 replies
-
|
Hey @abreumatheus, as you've said - we did a quick fix when we first made this fork we exposed them as variables in the fetcher classes, here, here and here - So I hope that helps as an intermediate solution. We were undecided whether or not to handle the throttling ourselves, there was a discussion around who has the responsibilities of this in the original fork. It seems that we're having more conversations around this again so I'll raise it with the other maintainer @JOJ0 and see what we think, and get back to you asap. Hope this is okay. Thanks |
Beta Was this translation helpful? Give feedback.
-
|
Hi @abreumatheus, thanks for mentioning this topic here. We are aware of it but haven't gotten around to discussing what we think is the right direction to go with it. BTW people hitting rate limits is one of the most reported topics on the Discogs forum too :-) @alifhughes and me now took some minutes to talk about it again and decided that we should try to find the people that originally filed PR's targeting rate-limit handling. This one, originally submitted by @hiaselhans in June 2019 looks promising: @harababurel also submitted a PR to the original Discogs repo in April 2020, it makes use of the third party module "backoff" (https://github.com/litl/backoff): I haven't tried both of the solutions myself and can't really tell which one is the better one but @alifhughes and me also discussed that maybe it should even be possible that the rate-limiting solution should be configurable on initialisation of the client class. Give us your thoughts and if someone finds the time to try out one of those suggested PR's, it would help us a lot. So as already said @hiaselhans and @abreumatheus, if you are still around, we are welcoming you to submit your PR's and let's work on them together :-) Thanks a lot in advance! |
Beta Was this translation helpful? Give feedback.
-
|
Hi all! It's already quite some time ago but it seems like i still have my fork. Let me know once you have decided and i could rebase and file the PR here. Have Fun! |
Beta Was this translation helpful? Give feedback.
-
|
Hi @hiaselhans thanks for getting back to us so quickly! It would be wonderful if you'd file the PR and we work from there. Probably there'll be slight changes necessary. This will help us a lot! Thanks in advance. Great! |
Beta Was this translation helpful? Give feedback.
-
|
Hi guys, sorry for the delay on my side. I paticurlaly liked the @harababurel a little bit more, seems like the simpler it can get. Maybe the only modification would be expose a property like the |
Beta Was this translation helpful? Give feedback.
-
I agree, I think @JOJ0 will too. I will get a PR open with this implementation anyway and we can discuss it further there, if we implement it and decide to go on a slightly different route, the PR will be the right way to capture that. I'll edit this question in the future with a link to the PR so that people will know where to go. Thanks for the good discussion guys, feel free to mark this as unanswered if we have more to say! |
Beta Was this translation helpful? Give feedback.
-
|
Actually I liked @hiaselhans more because it does not pull in another module, even though I think backoff seems to be a very well maintained package, but still: the best way would be if the user can choose betwee: no rate-limit-handling, backoff, builtin (hiaselhans) version. something like that. let's wait until @alifhughes and @hiaselhans do the PR's and then think about how to make it configurable. at least it would be good to get any of the two into the code. |
Beta Was this translation helpful? Give feedback.
-
|
@JOJ0 surely, rate-limit protection should just be enforced, no one wants to get 429's |
Beta Was this translation helpful? Give feedback.
-
|
@VinylVault I think that is making an assumption of our users of the client. You could argue we could only be a layer over the API and not offer much more of our own functionality on top of it (which was argued in a PR against the original repo). However I think we're moving towards more than that. So it doesn't hurt for us to enable it by default and then offer the user of the client to not allow it if they wish. It is win win. I made a start to the development of this over the weekend but a bit short on time at the moment. Hope to have it done soon. |
Beta Was this translation helpful? Give feedback.
-
|
@alifhughes I agree that there are advantages and disadvantages to default ON, but to me, it's always better to cater to the learners / less experienced users of a language, than expecting that everyone can handle the error out of the box. Part of the reason I would like to see that, is purely selfish, I use a specific project as my gateway project to learn a language, but doing that, enables me to see where there are potential issues with any module / helper, that other people may be struggling with, but don't want to raise an issue for the fear of looking like a dummy ... me I don't care about looking stupid, I know I'm learning :D |
Beta Was this translation helpful? Give feedback.
-
|
Just wanted to post an update that I haven't forgotten about this thread or the rate limiting, I have a work in progress, draft PR here that I think could be the solution. Hope to get it complete and ready for review asap. 🙂 |
Beta Was this translation helpful? Give feedback.
-
|
any news @alifhughes |
Beta Was this translation helpful? Give feedback.
-
|
As PR #34 will be merged sooner or later I will mark this question as answered :-) |
Beta Was this translation helpful? Give feedback.
-
|
This has now been released in version 2.3.10 - link to download on PyPI here |
Beta Was this translation helpful? Give feedback.
As PR #34 will be merged sooner or later I will mark this question as answered :-)