Repository: stravalib/stravalib
Pull request #520 (2024-06-07 03:51:04)

Checks for activities/id endpoint 🙈 
Addresses in part #506

lwasser: Ok - this is also ready for review. there is a part 2 - to this pr where we take a handful of missing and renamed objects related to gear, etc and fix them as well. i'm hesitant to do that in this pr - rather i'd prefer a solid review, we can evaluate the need for additional tests and merge and then work on the other new objects separately. some of them are related to segment returns, gear, athlete etc.
lwasser: mypy is still unhappy however i'm not sure given the other issues we have open if that is critical to fix - NOW vs addressing the rest of the endpoints and issues that might resolve some of the type issues naturally. but please say the word if we should focus on mypy passing now - i will just need a bit of guidance if that is the case. 
jsamoocha: We first need to merge #517 and #519 and rebase this branch on those changes. It's good to rename the existing `model.Activity` to `DetailedActivity` for consistency with the Strava API. However, we cannot simply change all the existing references from `model.Activity` to `model.DetailedActivity`. Some of those should be `ClubActivity`, `SummaryActivity`, or `MetaActivity`.
lwasser: this makes sense @jsamoocha i'm going to turn this into a draft again. i'll get the other pr #519 cleaned up for another review. and we can go from there!
lwasser: > Finally, I addressed your comment about the naming of `ActivityLap`, and corrected the type of `Lap.activity`.
> 
> Almost there 😅

thank you @jsamoocha so... before i started working on this again i rebased and force pushed 🙈 I think you added some commits to fix Lap.activity. and i am not sure that i can retrieve those after a force push. can you either re-add what you implemented or tell me what you wrote / did?

my apologies!! i should have `git pulled` first before rebasing, it's just habit to rebase and start working again after a pr has been merged. i don't think i can use reflog in this case because of the order of how i did things. 

i've addressed everything else. (i think?!) 
lwasser: Ok i was able to retrieve the commits visually using email links. i tried to cherry pick the commit itself as i could see it from my email link on github but i've already overwritten it with the force push so git didn't like be trying to cherry pick. i actually don't know how that works and it's hurting my brain to think about so i've just added the changes myself!  🤯 

hopefully this addresses all concerns. this change really just removes some of the stuff i added that was extraneous and puts back those (very cool to have) lazy properties that i didn't understand before. 


Patch:
diff --git a/src/stravalib/client.py b/src/stravalib/client.py
index e141de2..0a9993d 100644
--- a/src/stravalib/client.py
+++ b/src/stravalib/client.py
@@ -526,6 +526,10 @@ class Client:
         -------
         class: `model.DetailedClub` object containing the club data.
 
+        Notes
+        ------
+
+        https://developers.strava.com/docs/reference/#api-Clubs-getClubById
         """
         raw = self.protocol.get("/clubs/{id}", id=club_id)
         return model.DetailedClub.model_validate(
@@ -580,7 +584,7 @@ class Client:
         Returns
         -------
         class:`BatchedResultsIterator`
-            An iterator of :class:`stravalib.model.Activity` objects.
+            An iterator of :class:`stravalib.model.ClubActivity` objects.
 
         """
         result_fetcher = functools.partial(
@@ -628,7 +632,7 @@ class Client:
 
     def get_activity(
         self, activity_id: int, include_all_efforts: bool = False
-    ) -> model.Activity:
+    ) -> model.DetailedActivity:
         """Gets specified activity.
 
         Will be detail-level if owned by authenticated user; otherwise
@@ -646,7 +650,7 @@ class Client:
 
         Returns
         -------
-        class: `model.Activity`
+        class: `model.DetailedActivity`
             An Activity object containing the requested activity data.
 
         """
@@ -655,7 +659,9 @@ class Client:
             id=activity_id,
             include_all_efforts=include_all_efforts,
         )
-        return model.Activity.model_validate({**raw, **{"bound_client": self}})
+        return model.DetailedActivity.model_validate(
+            {**raw, **{"bound_client": self}}
+        )
 
     def _validate_activity_type(
         self,
@@ -675,10 +681,10 @@ class Client:
             Deprecated. Prefer to use sport_type. In a request where both type
             and sport_type are present, this field will be ignored.
             See https://developers.strava.com/docs/reference/#api-models-UpdatableActivity.
-            For possible values see: :class:`stravalib.model.Activity.TYPES`
+            For possible values see: :class:`stravalib.model.DetailedActivity.TYPES`
         sport_type : str, default=None
             For possible values (case-sensitive) see:
-            :class:`stravalib.model.Activity.SPORT_TYPES`
+            :class:`stravalib.model.DetailedActivity.SPORT_TYPES`
 
         Returns
         -------
@@ -694,9 +700,9 @@ class Client:
         # Check for sport_type first. If provided, it is favored over
         # activity_type / type
         if sport_type is not None:
-            if not sport_type in model.Activity.SPORT_TYPES:
+            if not sport_type in model.DetailedActivity.SPORT_TYPES:
                 raise ValueError(
-                    f"Invalid activity type: {sport_type}. Possible values: {model.Activity.SPORT_TYPES!r}"
+                    f"Invalid activity type: {sport_type}. Possible values: {model.DetailedActivity.SPORT_TYPES!r}"
                 )
             params["sport_type"] = sport_type
             return params
@@ -706,10 +712,10 @@ class Client:
         # sport_type.
         elif activity_type is not None:
             if not activity_type.lower() in [
-                t.lower() for t in model.Activity.TYPES
+                t.lower() for t in model.DetailedActivity.TYPES
             ]:
                 raise ValueError(
-                    f"Invalid activity type: {activity_type}. Possible values: {model.Activity.TYPES!r}"
+                    f"Invalid activity type: {activity_type}. Possible values: {model.DetailedActivity.TYPES!r}"
                 )
             params["type"] = activity_type.lower()
             warn_param_deprecation(
@@ -720,8 +726,6 @@ class Client:
 
         return params
 
-    # TODO: according to the strava api docs we can add an optional trainer
-    # and commute param here with boolean 0/1 values
     def create_activity(
         self,
         name: str,
@@ -731,7 +735,9 @@ class Client:
         activity_type: ActivityType | None = None,
         description: str | None = None,
         distance: pint.Quantity | float | None = None,
-    ) -> model.Activity:
+        trainer: bool | None = None,
+        commute: bool | None = None,
+    ) -> model.DetailedActivity:
         """Create a new manual activity.
 
         If you would like to create an activity from an uploaded GPS file, see the
@@ -746,10 +752,10 @@ class Client:
             Deprecated. Prefer to use sport_type. In a request where both type
             and sport_type are present, this field will be ignored.
             See https://developers.strava.com/docs/reference/#api-models-UpdatableActivity.
-            For possible values see: :class:`stravalib.model.Activity.TYPES`
+            For possible values see: :class:`stravalib.model.DetailedActivity.TYPES`
         sport_type : str, default=None
             For possible values (case-sensitive) see: For possible values
-            see: :class:`stravalib.model.Activity.SPORT_TYPES`
+            see: :class:`stravalib.model.DetailedActivity.SPORT_TYPES`
         start_date_local : class:`datetime.datetime` or string in ISO8601 format
             Local date/time of activity start. (TZ info will be ignored)
         elapsed_time : class:`datetime.timedelta` or int (seconds)
@@ -758,7 +764,15 @@ class Client:
             The description for the activity.
         distance : class:`pint.Quantity` or float (meters), default=None
             The distance in meters (float) or a :class:`pint.Quantity` instance.
+        trainer : bool
+            Whether this activity was completed using a trainer (or not)
+        commute : bool
+            Whether the activity is a commute or not.
 
+        Notes
+        -----
+        See: https://developers.strava.com/docs/reference/#api-Uploads-createUpload
+        See: https://developers.strava.com/docs/reference/#api-Activities-createActivity
         """
 
         # Strava API requires either sport_type or activity_type to be defined
@@ -783,11 +797,16 @@ class Client:
             elapsed_time=elapsed_time,
         )
 
-        if description is not None:
-            params["description"] = description
+        update_params = {
+            "description": description,
+            "distance": distance,
+            "commute": commute,
+            "trainer": trainer,
+        }
 
-        if distance is not None:
-            params["distance"] = distance
+        for key, value in update_params.items():
+            if value is not None:
+                params[key] = value
 
         params = self._validate_activity_type(
             params, activity_type, sport_type
@@ -795,7 +814,7 @@ class Client:
 
         raw_activity = self.protocol.post("/activities", **params)
 
-        return model.Activity.model_validate(
+        return model.DetailedActivity.model_validate(
             {**raw_activity, **{"bound_client": self}}
         )
 
@@ -812,11 +831,9 @@ class Client:
         gear_id: int | None = None,
         device_name: str | None = None,
         hide_from_home: bool | None = None,
-    ) -> model.Activity:
+    ) -> model.DetailedActivity:
         """Updates the properties of a specific activity.
 
-        https://developers.strava.com/docs/reference/#api-Activities-updateActivityById
-
         Parameters
         ----------
         activity_id : int
@@ -828,9 +845,9 @@ class Client:
             Deprecated. Prefer to use sport_type. In a request where both type
             and sport_type are present, this field will be ignored.
             See https://developers.strava.com/docs/reference/#api-models-UpdatableActivity.
-            For possible values see: :class:`stravalib.model.Activity.TYPES`
+            For possible values see: :class:`stravalib.model.DetailedActivity.TYPES`
         sport_type : str, default=None
-            For possible values see: :class:`stravalib.model.Activity.SPORT_TYPES`
+            For possible values see: :class:`stravalib.model.DetailedActivity.SPORT_TYPES`
         private : bool, default=None
             Whether the activity is private.
             .. deprecated:: 1.0
@@ -855,6 +872,11 @@ class Client:
         Returns
         -------
             Updates the activity in the selected Strava account
+
+        Notes
+        ------
+        See: https://developers.strava.com/docs/reference/#api-Activities-updateActivityById
+
         """
 
         # Convert the kwargs into a params dict
@@ -895,7 +917,7 @@ class Client:
             "/activities/{activity_id}", activity_id=activity_id, **params
         )
 
-        return model.Activity.model_validate(
+        return model.DetailedActivity.model_validate(
             {**raw_activity, **{"bound_client": self}}
         )
 
@@ -978,10 +1000,10 @@ class Client:
             params["description"] = description
         if activity_type is not None:
             if not activity_type.lower() in [
-                t.lower() for t in model.Activity.TYPES
+                t.lower() for t in model.DetailedActivity.TYPES
             ]:
                 raise ValueError(
-                    f"Invalid activity type: {activity_type}. Possible values: {model.Activity.TYPES!r}"
+                    f"Invalid activity type: {activity_type}. Possible values: {model.DetailedActivity.TYPES!r}"
                 )
             warn_param_unofficial("activity_type")
             params["activity_type"] = activity_type.lower()
@@ -1160,7 +1182,7 @@ class Client:
 
     def get_activity_laps(
         self, activity_id: int
-    ) -> BatchedResultsIterator[model.ActivityLap]:
+    ) -> BatchedResultsIterator[model.Lap]:
         """Gets the laps from an activity.
 
         https://developers.strava.com/docs/reference/#api-Activities-getLapsByActivityId
@@ -1181,7 +1203,7 @@ class Client:
         )
 
         return BatchedResultsIterator(
-            entity=model.ActivityLap,
+            entity=model.Lap,
             bind_client=self,
             result_fetcher=result_fetcher,
         )
@@ -2225,7 +2247,7 @@ class ActivityUploader:
 
     def wait(
         self, timeout: float | None = None, poll_interval: float = 1.0
-    ) -> model.Activity:
+    ) -> model.DetailedActivity:
         """Wait for the upload to complete or to err out.
 
         Will return the resulting Activity or raise an exception if the
@@ -2242,7 +2264,7 @@ class ActivityUploader:
 
         Returns
         -------
-        class:`stravalib.model.Activity`
+        class:`stravalib.model.DetailedActivity
 
         Raises
         ------
diff --git a/src/stravalib/model.py b/src/stravalib/model.py
index 698089c..3dbb15b 100644
--- a/src/stravalib/model.py
+++ b/src/stravalib/model.py
@@ -45,7 +45,6 @@ from stravalib.strava_model import (
     Comment,
     DetailedGear,
     ExplorerSegment,
-    Lap,
     LatLng,
     PhotosSummary,
     PolylineMap,
@@ -347,9 +346,6 @@ class SummaryClub(MetaClub, strava_model.SummaryClub):
     profile: Optional[str] = None
     description: Optional[str] = None
     club_type: Optional[str] = None
-    activity_types_icon: Optional[str] = None
-    dimensions: Optional[list[str]] = None
-    localized_sport_type: Optional[str] = None
 
     @lazy_property
     def members(self) -> BatchedResultsIterator[Athlete]:
@@ -365,7 +361,7 @@ class SummaryClub(MetaClub, strava_model.SummaryClub):
         return self.bound_client.get_club_members(self.id)
 
     @lazy_property
-    def activities(self) -> BatchedResultsIterator[Activity]:
+    def activities(self) -> BatchedResultsIterator[ClubActivity]:
         """
         Lazy property to retrieve club activities.
 
@@ -434,9 +430,11 @@ class AthleteStats(strava_model.ActivityStats):
 
 
 class MetaAthlete(strava_model.MetaAthlete, BoundClientEntity):
-    pass
+    # Undocumented
+    resource_state: Optional[int] = None
 
 
+# TODO: rename to detailed athlete
 class Athlete(strava_model.DetailedAthlete):
     """Represents high level athlete information including
     their name, email, clubs they belong to, bikes, shoes, etc.
@@ -650,12 +648,12 @@ class ActivityPhoto(BaseModel):
         )
 
 
-class ActivityLap(
-    Lap,
+class Lap(
+    strava_model.Lap,
     BoundClientEntity,
 ):
     # Field overrides from superclass for type extensions:
-    activity: Optional[Activity] = None
+    activity: Optional[MetaActivity] = None
     athlete: Optional[Athlete] = None
 
     # Undocumented attributes:
@@ -915,8 +913,7 @@ class SummaryActivity(MetaActivity, strava_model.SummaryActivity):
     )(check_valid_location)
 
 
-# TODO: should this be named Detailed Activity?
-class Activity(
+class DetailedActivity(
     SummaryActivity,
     strava_model.DetailedActivity,
 ):
@@ -925,13 +922,18 @@ class Activity(
     """
 
     # field overrides from superclass for type extensions:
+    # TODO: should be SummaryGear - check returns
     gear: Optional[Gear] = None
-    # Ignoring types here given there are overrides
     best_efforts: Optional[list[BestEffort]] = None  # type: ignore[assignment]
+    # TODO: returning empty list should be  DetailedSegmentEffort object
+    # TODO: test on activity with actual segments
     segment_efforts: Optional[list[SegmentEffort]] = None  # type: ignore[assignment]
+    # TODO: Returns Split object - check returns for that object
     splits_metric: Optional[list[Split]] = None  # type: ignore[assignment]
     splits_standard: Optional[list[Split]] = None  # type: ignore[assignment]
+    # TODO: should be PhotosSummary
     photos: Optional[ActivityPhotoMeta] = None
+    # TODO: check Lap returns against spec
     laps: Optional[list[Lap]] = None
 
     # Added for backward compatibility
@@ -965,6 +967,9 @@ class Activity(
     from_accepted_tag: Optional[bool] = None
     segment_leaderboard_opt_out: Optional[bool] = None
     perceived_exertion: Optional[int] = None
+    prefer_perceived_exertion: Optional[bool] = None
+    visibility: Optional[str] = None
+    private_note: Optional[str] = None
 
     _naive_local = field_validator("start_date_local")(naive_datetime)
 


Test patch:
diff --git a/src/stravalib/tests/unit/test_model.py b/src/stravalib/tests/unit/test_model.py
index 1095758..5f1eb8b 100644
--- a/src/stravalib/tests/unit/test_model.py
+++ b/src/stravalib/tests/unit/test_model.py
@@ -1,19 +1,18 @@
-from datetime import datetime, timedelta, timezone
+from datetime import datetime, timezone
 
 import pytest
 from dateutil.parser import ParserError
 
 from stravalib import model
 from stravalib.model import (
-    Activity,
-    ActivityLap,
     ActivityPhoto,
     ActivityTotals,
     AthletePrEffort,
     AthleteSegmentStats,
     AthleteStats,
     BaseEffort,
-    # Club, # CLUB IS not currently used in this module
+    DetailedActivity,
+    Lap,
     LatLon,
     RelaxedActivityType,
     RelaxedSportType,
@@ -33,33 +32,33 @@ from stravalib.tests import TestBase
     "model_class,raw,expected_value",
     (
         (
-            Activity,
+            DetailedActivity,
             {"start_latlng": []},
             None,
         ),
         (
-            Activity,
+            DetailedActivity,
             {"end_latlng": []},
             None,
         ),
         (
-            Activity,
+            DetailedActivity,
             {"end_latlng": "5.4,4.3"},
             LatLon([5.4, 4.3]),
         ),
         (
-            Activity,
+            DetailedActivity,
             {"start_latlng": "5.4,4.3"},
             LatLon([5.4, 4.3]),
         ),
-        (Activity, {"start_latlng": []}, None),
+        (DetailedActivity, {"start_latlng": []}, None),
         (Segment, {"start_latlng": []}, None),
         (SegmentExplorerResult, {"start_latlng": []}, None),
         (ActivityPhoto, {"location": []}, None),
         # TODO re-add this Activity test when custom types are implemented
-        # (Activity, {"timezone": "foobar"}, None),
+        # (DetailedActivity, {"timezone": "foobar"}, None),
         (
-            Activity,
+            DetailedActivity,
             {"start_date_local": "2023-01-17T11:06:07Z"},
             datetime(2023, 1, 17, 11, 6, 7),
         ),
@@ -69,7 +68,7 @@ from stravalib.tests import TestBase
             datetime(2023, 1, 17, 11, 6, 7),
         ),
         (
-            ActivityLap,
+            Lap,
             {"start_date_local": "2023-01-17T11:06:07Z"},
             datetime(2023, 1, 17, 11, 6, 7),
         ),
@@ -145,10 +144,10 @@ def test_subscription_callback_field_names():
 @pytest.mark.parametrize(
     "klass,attr,given_type,expected_type",
     (
-        (Activity, "sport_type", "Run", "Run"),
-        (Activity, "sport_type", "FooBar", "Workout"),
-        (Activity, "type", "Run", "Run"),
-        (Activity, "type", "FooBar", "Workout"),
+        (DetailedActivity, "sport_type", "Run", "Run"),
+        (DetailedActivity, "sport_type", "FooBar", "Workout"),
+        (DetailedActivity, "type", "Run", "Run"),
+        (DetailedActivity, "type", "FooBar", "Workout"),
         (Segment, "activity_type", "Run", "Run"),
         (Segment, "activity_type", "FooBar", "Workout"),
     ),
@@ -193,12 +192,12 @@ class VelocityType:
         (ActivityTotals, "moving_time", int, TimeDeltaType),
         (AthleteStats, "biggest_ride_distance", float, DistanceType),
         (AthleteStats, "biggest_climb_elevation_gain", float, DistanceType),
-        (ActivityLap, "distance", float, DistanceType),
-        (ActivityLap, "total_elevation_gain", float, DistanceType),
-        (ActivityLap, "average_speed", float, VelocityType),
-        (ActivityLap, "max_speed", float, VelocityType),
-        (ActivityLap, "elapsed_time", int, TimeDeltaType),
-        (ActivityLap, "moving_time", int, TimeDeltaType),
+        (Lap, "distance", float, DistanceType),
+        (Lap, "total_elevation_gain", float, DistanceType),
+        (Lap, "average_speed", float, VelocityType),
+        (Lap, "max_speed", float, VelocityType),
+        (Lap, "elapsed_time", int, TimeDeltaType),
+        (Lap, "moving_time", int, TimeDeltaType),
         (Split, "distance", float, DistanceType),
         (Split, "elevation_difference", float, DistanceType),
         (Split, "average_speed", float, VelocityType),
@@ -218,13 +217,13 @@ class VelocityType:
         (BaseEffort, "distance", float, DistanceType),
         (BaseEffort, "elapsed_time", int, TimeDeltaType),
         (BaseEffort, "moving_time", int, TimeDeltaType),
-        (Activity, "distance", float, DistanceType),
-        (Activity, "timezone", str, TimezoneType),
-        (Activity, "total_elevation_gain", float, DistanceType),
-        (Activity, "average_speed", float, VelocityType),
-        (Activity, "max_speed", float, VelocityType),
-        (Activity, "elapsed_time", int, TimeDeltaType),
-        (Activity, "moving_time", int, TimeDeltaType),
+        (DetailedActivity, "distance", float, DistanceType),
+        (DetailedActivity, "timezone", str, TimezoneType),
+        (DetailedActivity, "total_elevation_gain", float, DistanceType),
+        (DetailedActivity, "average_speed", float, VelocityType),
+        (DetailedActivity, "max_speed", float, VelocityType),
+        (DetailedActivity, "elapsed_time", int, TimeDeltaType),
+        (DetailedActivity, "moving_time", int, TimeDeltaType),
         (Route, "distance", float, DistanceType),
         (Route, "elevation_gain", float, DistanceType),
     ),
@@ -247,9 +246,9 @@ class ModelTest(TestBase):
 
         Notes
         -----
-        In Pydantic 2.x we use model_validate instead of parse_object.
-        Model_Validate always returns a new model. so in this test we
-        instantiate a new instance a when calling model_validate aligning with
+        In Pydantic 2.x we use `model_validate` instead of `parse_object`.
+        Model_Validate always returns a new model. In this test we
+        instantiate a new instance a when calling `model_validate` aligning with
         Pydantic's immutability approach.
 
         """
@@ -302,11 +301,7 @@ class ModelTest(TestBase):
 @pytest.mark.parametrize(
     "input_value, expected_output, exception",
     [
-        (
-            "2024-04-28T12:00:00Z",
-            datetime(2024, 4, 28, 12, 0),
-            None
-        ),
+        ("2024-04-28T12:00:00Z", datetime(2024, 4, 28, 12, 0), None),
         (
             int(datetime(2022, 4, 28, 12, 0).timestamp()),
             datetime(2022, 4, 28, 12, 0),