Skip to content

Conversation

@chuan137
Copy link
Member

No description provided.

@chuan137 chuan137 requested a review from Carthaca August 20, 2024 11:26
@chuan137 chuan137 force-pushed the feat-custom-ratelimit branch from 3084b72 to e899c0c Compare August 20, 2024 12:00
@sven-rosenzweig
Copy link
Contributor

Hi @chuan137 ,
Just stumbled around your PR.
From what I understood you are trying to add a custom section (besides global, defaults) to fetch rate limits from, right?

Generally I am missing the following:

  • Description in this PR would be nice
  • Extending the documentation
  • Unit Test proofing that your scenario is working

:return: the local rate limit or -1 if not set
"""
ttu_ratelimits = self.local_ratelimits.get(target_type_uri, [])
_ratelimits = self.custom_ratelimits.get(scope, None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are custom limits fetched via the get_global_rate_limits? Could it be part of the get_local_rate_limits?
If yes, we could thing about merging def get_local_rate_limits(self, scope, action, target_type_uri, **kwargs) and f get_global_rate_limits(self, action, target_type_uri, **kwargs) into a single method. Those are nearly identical.

 # Get local (for a certain scope) rate limits from provider.
        local_rate_limit = self.ratelimit_provider.get_local_rate_limits(
            scope, action, trimmed_target_type_uri
        )

move to

 # Get local (for a certain scope) rate limits from provider.
        local_rate_limit = self.ratelimit_provider.get_rate_limits(
            scope, action, trimmed_target_type_uri, 'local')
        ...
        custom_rate_limit = self.ratelimit_provider.get_rate_limits(
            scope, action, trimmed_target_type_uri, '<custom>')

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.

3 participants