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

added ratelimit - not tested #39

Closed
wants to merge 1 commit into from
Closed

Conversation

cryguy
Copy link

@cryguy cryguy commented Sep 28, 2017

added ratelimit, might need fixing , not the cleanest

added ratelimit, might need fixing , not the cleanest
@cryguy cryguy mentioned this pull request Sep 28, 2017
@codingo
Copy link
Owner

codingo commented Sep 28, 2017

@cryguy We just had two pr's for this (in a space of 20m!). Could you take a look at the pr from @grimd34th and let me know if you feel there's an edge case your PR could handle that wouldn't be managed by #40?

At the moment I'm leaning towards #40 due to the lower dependencies / code base but I'm happy to be persuaded if this is functionality more flexible. Still going to add you to /CREDITS either way!

@cryguy
Copy link
Author

cryguy commented Sep 28, 2017

I feel that my method is better? As some apis allow for 100 runs in an hour, the code can be modified to allow for that

@cryguy
Copy link
Author

cryguy commented Sep 28, 2017

Im new to this and english is not my first language so im sorry if the grammar is bad :)

@codingo
Copy link
Owner

codingo commented Sep 28, 2017

Potentially that's an edge case - I'll have to read into it more. No dramas about the grammar, code speaks :) I'm going hold back on this for a while to give @grimd34th a chance to respond.

Happy to pull these both down into VM's and test them over the weekend in different scenarios as well. If we do go with this option I'll probably split it out into a new helper class to make it a bit easier to manage over the longer term.

@cryguy
Copy link
Author

cryguy commented Sep 28, 2017

:) happy to help

@grimd34th
Copy link
Contributor

I suppose it is up to @codingo, based on how many dependencies he wants to introduce and code complexity increases. My solution can have the same outcome as cryguy's with mine being at the end of every request instead of spaced throughout, so its up to the user to know the time they want to sleep after each request.

@codingo
Copy link
Owner

codingo commented Sep 29, 2017

Not ignoring either of these - going to have to build out a new test case for them so will review in 24 hours.

@codingo
Copy link
Owner

codingo commented Sep 30, 2017

I'm going to close this thread - for a few reasons.

Primarily both @timkent and I want to limit the dependencies we have within the application to keep it as lightweight as possible. I find @grimd34th's solution to be the better of the two here as I was also unable to find any use case where this branch would provide functionality above @grimd34th's.

I also took a bit of an issue with code being directly copied (https://github.com/tomasbasham/ratelimit/blob/master/ratelimit/__init__.py) without a citation. In future commits I'd suggest also citing where any significant blocks of code have come from so we can add it to /CREDITS. I'm a lover of open source, but I also think it's very important to cite your sources and give credit where credit due.

All of that aside, thank-you for taking the time. There's still a lot of other issues remaining in this project if you want to throw your hat in again!

@codingo codingo closed this Sep 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants