-
Notifications
You must be signed in to change notification settings - Fork 35
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
Pyrate limiter v3 #111
Pyrate limiter v3 #111
Conversation
e677684
to
3933126
Compare
Sure, I'd be happy to review this. I should have some time this weekend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't finished looking over all the code changes yet, but I've reviewed the documentation. I think you've done a good job with this! Here are a few suggestions:
Readme intro
I'd recommend adding a Quickstart section immediately after the Installation section.
There will be two main categories of people looking at a readme (or docs in general):
- People coming directly from a google search, blog link, etc. who are just quickly skimming over the project to see if it's something they're interested in
- People who are already using the library and want to know more about how to use it
The first group is generally going to have a very short attention span, so one of the first things they should see is a minimal example they can copy & paste to quickly test it out. Something like, "given a user function my_function()
, here's how to limit its rate to x calls per minute." After trying that out, then the user can refer to the subsequent sections to learn more about the rest of the library's features. The requests library docs are a good example of this.
Topic organization
For the rest of the documentation, it may make navigation a bit easier to split it up into a small number (say, 3-5) of top-level sections. After the intro sections, I think most of the content could be divided broadly into "basic usage" and "advanced usage."
For example, the diagram you made under Key Components is great for visualizing a more advanced use case, but if that's the first thing a new/prospective user sees, they might get the wrong impression that the library is more complicated than what they need.
An overall outline of the readme, then, could look something like:
* Introduction
* Features
* Installation
* Quickstart
* Basic usage
* Key concepts
* Defining rate limits
* Wrapping all up with Limiter
* Handling exceeded limits
* Backends
* Advanced usage
* (Diagram showing Limiter with BucketFactory and multiple buckets could go here)
* Time sources
* Routing with multiple buckets
* Weights
* Async decorator/contextmanager
* Custom leaking behavior
* Custom backends
That's just one way to organize it, of course, and you don't have to use that verbatim, but that might give you some ideas. My main overall suggestion is to think of different types of audience (brand new to library / basic user / advanced user) and imagine how they would navigate to the kinds of information they're most likely looking for.
Docstrings
It's very useful to have full parameter info in the API Reference docs. An advanced user will often go directly there to find out how to use the library, and that info is also visible in an IDE, interpreter, Jupyter, etc. I'd recommend adding that to class docstrings. Type information is automatically pulled from type annotations (via the sphinx-autodoc-typehints
extension).
Example: docstring + signature and generated HTML docs.
I can help with this and any other Sphinx-specific parts, if you'd like.
API
A couple small suggestions for the API:
- Make all public classes and functions importable from the top-level package, so users can import like this:
from pyrate_limiter import Duration, Rate, InMemoryBucket, Limiter
- Since the
allowed_delay
parameter forLimiter
is a maximum value, I think keeping the name asmax_delay
would be clearer. "Allowed" could potentially be interpreted as "minimum," "maximum," or "exact." - I imagine the majority of users will most likely use the in-memory bucket, so you might consider making
bucket_factory
an optional parameter and default toInMemoryBucket
.
Thanks for review, I'll make the changes accordingly |
@JWCook I have made changes according to your suggestions. The docstring work still remains tho. I hope we can release this new version by the end of August - that would be pretty sweet. For the docstring maybe we can gradually roll them out with subsequent releases? Or maybe you can lend a hand with that if possible? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Thanks for making those changes.
I can help with updating the docstrings and reviewing the Sphinx docs. I will probably have time to get to that by the end of this week. I added #120 for this.
61a60a3
to
08431a1
Compare
Phase I release
Refactor
allowed_delay
tomax_delay
#118