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

Draft: adds rate limit class #40

Closed
wants to merge 9 commits into from

Conversation

simonneutert
Copy link
Collaborator

this may not be exactly what you had in mind, but maybe we can make this work in a way you feel good merging it. let me know what you think and I will add tests and finish the job.

@simonneutert simonneutert changed the title adds rate limit class Draft: adds rate limit class Jul 18, 2021
@@ -15,6 +15,8 @@ class Client < Strava::Web::Client

attr_accessor(*Config::ATTRIBUTES)

attr_accessor :ratelimit_status
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added the RatelimitStatus class to the client, making it possible to check the limits after each call. Added a boolean flag, too, as not every call gets rate limit in its headers.

Copy link
Owner

@dblock dblock left a comment

Choose a reason for hiding this comment

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

It's a good start. Keeping state on the client will make it definitely not threadsafe, so we shouldn't. Instead, wrap response.body with Response.new(response) and implement rate limit headers on that. Make sure Response#to_s returns response.body. And generally this is more future-proof for other extracts from .headers.

@@ -0,0 +1,59 @@
module Strava
module Api
class RatelimitStatus
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should call this Ratelimit , to match the headers, or RatelimitHeaders.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i pushed a bit to where it's heading at, please give feedback and I will see where this is going or whether it's going to be a dead end.

similar to paginate model extraction/instantiation is now backend with ratelimits like

def enrich_model_with_headers(model, response)
        response.ratelimit_headers
        model._response_headers = response.headers
        model._response_ratelimit = response.ratelimit_headers
        model
end

duplicating the "body" into the model via Response is not worth it, I'd say ...

lib/strava/api/ratelimit_status.rb Outdated Show resolved Hide resolved
lib/strava/api/ratelimit_status.rb Outdated Show resolved Hide resolved
lib/strava/api/ratelimit_status.rb Outdated Show resolved Hide resolved
@simonneutert
Copy link
Collaborator Author

simonneutert commented Jul 18, 2021

errors go down with every endpoint I fix, but I still do have some busy work to finish.

but I'd like to get your feedback first. I do like that models do not get instantiated directly anymore, to be honest. it feels a bit cleaner having methods be in charge, just like paginate always did the magic.

@dblock
Copy link
Owner

dblock commented Jul 19, 2021

errors go down with every endpoint I fix, but I still do have some busy work to finish.

but I'd like to get your feedback first. I do like that models do not get instantiated directly anymore, to be honest. it feels a bit cleaner having methods be in charge, just like paginate always did the magic.

So you already see the problem, it's getting messy, so that's probably not the right solution. Try this:

  • The response is now an object.
  • Add a different constructor to the Model base class that takes a response. Now models will inherit from a class that is constructed from a response and not a hash, and it should keep a reference to the response object.
  • Because all models inherit from Model, you can get access to the response object and hence the headers.
  • Lazy instantiation everywhere so we only parse/construct when needed.

@simonneutert
Copy link
Collaborator Author

errors go down with every endpoint I fix, but I still do have some busy work to finish.
but I'd like to get your feedback first. I do like that models do not get instantiated directly anymore, to be honest. it feels a bit cleaner having methods be in charge, just like paginate always did the magic.

So you already see the problem, it's getting messy, so that's probably not the right solution. Try this:

  • The response is now an object.
  • Add a different constructor to the Model base class that takes a response. Now models will inherit from a class that is constructed from a response and not a hash, and it should keep a reference to the response object.
  • Because all models inherit from Model, you can get access to the response object and hence the headers.
  • Lazy instantiation everywhere so we only parse/construct when needed.

this was my first implementation I tried, but I failed getting it to work with the Hashie::Trash Model class inherits from

I will let this sink in a bit and see what my sub conscience comes up with 😞

@dblock
Copy link
Owner

dblock commented Jul 19, 2021

I will let this sink in a bit and see what my sub conscience comes up with 😞

Let me know if you need help!

@dblock
Copy link
Owner

dblock commented Dec 27, 2021

Want to try finish this up @simonneutert?

@simonneutert
Copy link
Collaborator Author

i will give this another try and get back to you with what I come up with ✌️

@dangerpr-bot
Copy link

1 Error
🚫 The TOC found in README.md doesn't match the sections of the file.
1 Warning
⚠️ Unless you're refactoring existing code or improving documentation, please update CHANGELOG.md.

Here's the expected TOC for README.md:

# Table of Contents

- [Installation](#installation)
- [Usage](#usage)
  - [Activities](#activities)
    - [Create an Activity](#create-an-activity)
    - [Get Activity](#get-activity)
    - [List Activity Photos](#list-activity-photos)
    - [List Activity Comments](#list-activity-comments)
    - [List Activity Kudoers](#list-activity-kudoers)
    - [List Activity Laps](#list-activity-laps)
    - [List Athlete Activities](#list-athlete-activities)
    - [Get Activity Zones](#get-activity-zones)
    - [Update Activity](#update-activity)
  - [Athletes](#athletes)
    - [Get Authenticated Athlete](#get-authenticated-athlete)
    - [Get Zones](#get-zones)
    - [Get Athlete Stats](#get-athlete-stats)
    - [Update Athlete](#update-athlete)
  - [Clubs](#clubs)
    - [List Club Activities](#list-club-activities)
    - [List Club Events](#list-club-events)
    - [List Club Administrators](#list-club-administrators)
    - [Get Club](#get-club)
    - [List Club Members](#list-club-members)
    - [List Athlete Clubs](#list-athlete-clubs)
  - [Gears](#gears)
    - [Get Equipment](#get-equipment)
  - [Routes](#routes)
    - [Export Route GPX](#export-route-gpx)
    - [Export Route TCX](#export-route-tcx)
    - [Get Route](#get-route)
    - [List Athlete Routes](#list-athlete-routes)
  - [Running Races](#running-races)
    - [Get Running Race](#get-running-race)
    - [List Running Races](#list-running-races)
  - [Segment Efforts](#segment-efforts)
    - [List Segment Efforts](#list-segment-efforts)
    - [Get Segment Effort](#get-segment-effort)
  - [Segments](#segments)
    - [Explore Segments](#explore-segments)
    - [Get Segment Leaderboard](#get-segment-leaderboard)
    - [List Starred Segments](#list-starred-segments)
    - [Get Segment](#get-segment)
    - [Star Segment](#star-segment)
  - [Streams](#streams)
    - [Get Activity Streams](#get-activity-streams)
    - [Get Segment Effort Streams](#get-segment-effort-streams)
    - [Get Segment Streams](#get-segment-streams)
  - [Uploads](#uploads)
    - [Upload Activity](#upload-activity)
    - [Get Upload](#get-upload)
  - [Pagination](#pagination)
  - [OAuth](#oauth)
    - [OAuth Workflow](#oauth-workflow)
    - [Deauthorize](#deauthorize)
    - [Command Line OAuth Workflow](#command-line-oauth-workflow)
  - [Webhooks](#webhooks)
- [Configuration](#configuration)
  - [Web Client Options](#web-client-options)
  - [API Client Options](#api-client-options)
  - [OAuth Client Options](#oauth-client-options)
  - [OAuth Client Ratelimits](#oauth-client-ratelimits)
  - [Webhooks Client Options](#webhooks-client-options)
- [Errors](#errors)
- [Tools](#tools)
  - [Strava OAuth Token](#strava-oauth-token)
- [Users](#users)
- [Resources](#resources)
- [Upgrading](#upgrading)
- [Contributing](#contributing)
- [Copyright and License](#copyright-and-license)

Here's an example of a CHANGELOG.md entry:

* [#40](https://github.com/dblock/strava-ruby-client/pull/40): Draft: adds rate limit class - [@simonneutert](https://github.com/simonneutert).

Generated by 🚫 Danger

@simonneutert
Copy link
Collaborator Author

new attempt in PR #50

This pull request was closed.
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