dblock / strava-ruby-client

A complete Ruby client for the Strava API v3.
https://code.dblock.org/2018/11/27/writing-a-new-strava-api-ruby-client.html
MIT License
97 stars 22 forks source link

Add support for descent #54

Open dblock opened 2 years ago

dblock commented 2 years ago

Descent is Ascent + (Start Elevation - End Elevation). Start Elevation and End Elevation can be obtained from AltitudeStream.

benCoomes commented 2 years ago

Hi @dblock I'd like to take a shot at this, as I need to learn Ruby and am familiar with the Strava API. However, I can't think of a good spot to put a total_elevation_loss method. The main problem is that calculating elevation loss requires data from two different endpoints (Stream for start and end elevation and Activity for total elevation gain). Are there any examples in the library of models generated through multiple resource requests? I couldn't find any.

I can see two options where the user is responsible for getting the supplemental data:

Which of these would fit best with the library? Or, is this probably something that users need to be responsible for computing?

dblock commented 2 years ago

I think we should think about it backwards. Let's start with the right interfaces at the model level first, then we can rethink where things go if they need to be in mixins. Mixins are just ways to reuse common functionality.

So, which model(s) need(s) total_elevation_loss? Start there and implement them in any way that works, and we can refactor into mixins later.

benCoomes commented 2 years ago

Okay, that makes sense. Just thinking about what each model class represents, activity_total, activity, explorer_segment, lap, route, segment, and split could all have a total_elevation_loss property. And, all except explorer_segement use the elevation mixin already. Since explorer_segement doesn't currently have any elevation properties, do you think they should all be added? I don't think adding descent by itself makes sense.

I'll start with the others though, and see what implementing a total_elevation_loss property looks like.

dblock commented 2 years ago

Okay, that makes sense. Just thinking about what each model class represents, activity_total, activity, explorer_segment, lap, route, segment, and split could all have a total_elevation_loss property. And, all except explorer_segement use the elevation mixin already. Since explorer_segement doesn't currently have any elevation properties, do you think they should all be added? I don't think adding descent by itself makes sense.

If they exist on that model, yes.

I'll start with the others though, and see what implementing a total_elevation_loss property looks like.

👍