Repository: stravalib/stravalib
Pull request #517 (2024-06-02 02:44:03)

Fix: fix return types - athlete clubs endpoint
This adjusts the return types for the athlete/clubs endpoint. It also updates a test and modifies the return object type to be an iterator vs a list. 

#506 





lwasser: i'm going to add endpoint docs for each one that i test in this pr. that way it's easy to maintain links - 

i can setup htmlproofer or lychee to run on our docs. that will find broken links. so if an endpoint link fails we will know about it. i'll turn this into a draft and will work on that here as well. 

Patch:
diff --git a/src/stravalib/client.py b/src/stravalib/client.py
index 71b2182..0bcbe6b 100644
--- a/src/stravalib/client.py
+++ b/src/stravalib/client.py
@@ -285,7 +285,7 @@ class Client:
             Result will start with activities whose start date is after
             specified value. (UTC)
         limit : int or None, default=None
-            How many maximum activities to return.
+            Maximum number of activities to return.
 
         Returns
         -------
@@ -451,26 +451,30 @@ class Client:
 
         return model.AthleteStats.model_validate(raw)
 
-    def get_athlete_clubs(self) -> list[model.Club]:
+    def get_athlete_clubs(
+        self, limit: int | None = None
+    ) -> BatchedResultsIterator[model.SummaryClub]:
         """List the clubs for the currently authenticated athlete.
 
         https://developers.strava.com/docs/reference/#api-Clubs-getLoggedInAthleteClubs
 
+        limit : int or None, default=None
+            Maximum number of club objects to return.
 
         Returns
         -------
-        :class:`list`
-            A list of :class:`stravalib.model.Club`
+        class:`BatchedResultsIterator`
+            An iterator of :class:`stravalib.model.SummaryClub` objects
 
         """
 
-        # TODO: This should return a BatchedResultsIterator or otherwise at
-        # most 30 clubs are returned!
-        club_structs = self.protocol.get("/athlete/clubs")
-        return [
-            model.Club.model_validate({**raw, **{"bound_client": self}})
-            for raw in club_structs
-        ]
+        result_fetcher = functools.partial(self.protocol.get, "/athlete/clubs")
+        return BatchedResultsIterator(
+            entity=model.SummaryClub,
+            bind_client=self,
+            result_fetcher=result_fetcher,
+            limit=limit,
+        )
 
     def join_club(self, club_id: int) -> None:
         """Joins the club on behalf of authenticated athlete.
diff --git a/src/stravalib/model.py b/src/stravalib/model.py
index e9c8cff..d391729 100644
--- a/src/stravalib/model.py
+++ b/src/stravalib/model.py
@@ -28,7 +28,13 @@ from typing import (
 )
 
 from dateutil import parser
-from pydantic import BaseModel, Field, field_validator, model_validator
+from pydantic import (
+    AliasChoices,
+    BaseModel,
+    Field,
+    field_validator,
+    model_validator,
+)
 
 from stravalib import exc, strava_model
 from stravalib.strava_model import (
@@ -39,7 +45,6 @@ from stravalib.strava_model import (
     Comment,
     DetailedClub,
     DetailedGear,
-    DetailedSegmentEffort,
     ExplorerSegment,
     Lap,
     LatLng,
@@ -47,10 +52,7 @@ from stravalib.strava_model import (
     PolylineMap,
     Primary,
     SportType,
-    SummaryClub,
     SummaryGear,
-    SummaryPRSegmentEffort,
-    SummarySegmentEffort,
     TimedZoneRange,
 )
 
@@ -141,7 +143,7 @@ def lazy_property(fn: Callable[[U], T]) -> Optional[T]:
 # Custom validators for some edge cases:
 
 
-# This is the first valid location check.
+# This method checks a list of floats - ie a stream not just a single lat/lon
 def check_valid_location(
     location: Optional[Union[list[float], str]]
 ) -> Optional[list[float]]:
@@ -253,13 +255,11 @@ class RelaxedSportType(SportType):
 
 class LatLon(LatLng):
     """
-    Enables backward compatibility for legacy namedtuple
+    Stores lat / lon values or None.
     """
 
-    # It seems like we are validating LatLon twice which is a bit confusing.
-    # Once here and once when we ingest lat long into the Activity Object.
     @model_validator(mode="before")
-    def check_valid_latlng(cls, values: list[float]) -> Optional[list[float]]:
+    def check_valid_latlng(cls, values: list[float]) -> list[float] | None:
         """Validate that Strava returned an actual lat/lon rather than an empty
         list. If list is empty, return None
 
@@ -274,12 +274,8 @@ class LatLon(LatLng):
         list or None
             list of lat/lon values or None
 
-        Notes
-        ------
-        This is the second validation of lat lon but it's a validator
-        the first is check is def check_valid_location above.
-
         """
+
         # Strava sometimes returns empty list in case of activities without GPS
         return values if values else None
 
@@ -325,7 +321,11 @@ class Club(
     Notes
     -----
     Clubs are the only object that can have multiple valid
-    `activity_types`. Activities only have one
+    `activity_types`. Activities only have one.
+
+    Endpoint docs are found here:
+    https://developers.strava.com/docs/reference/#api-models-SummaryClub
+
 
     """
 
@@ -361,6 +361,16 @@ class Club(
         return self.bound_client.get_club_activities(self.id)
 
 
+class SummaryClub(strava_model.SummaryClub):
+    """Represents the summaryClub object with undocumented attributes
+    included
+
+    """
+
+    # Undocumented attributes
+    profile: Optional[str] = None
+
+
 class Gear(DetailedGear):
     """
     Represents a piece of gear (equipment) used in physical activities.
@@ -708,24 +718,19 @@ class SegmentExplorerResult(
         return self.bound_client.get_segment(self.id)
 
 
-class AthleteSegmentStats(
-    SummarySegmentEffort,
-):
-    """
-    A structure being returned for segment stats for current athlete.
-    """
-
-    # Undocumented attributes:
-    effort_count: Optional[int] = None
-    pr_elapsed_time: Optional[timedelta] = None
-    pr_date: Optional[date] = None
-
-    _naive_local = field_validator("start_date_local")(naive_datetime)
-
-
 class AthletePrEffort(
-    SummaryPRSegmentEffort,
+    strava_model.SummaryPRSegmentEffort,
 ):
+    # Override fields from superclass to match actual responses by Strava API:
+    activity_id: Optional[int] = Field(
+        validation_alias=AliasChoices("pr_activity_id", "activity_id"),
+        default=None,
+    )
+    elapsed_time: Optional[int] = Field(
+        validation_alias=AliasChoices("pr_elapsed_time", "elapsed_time"),
+        default=None,
+    )
+
     # Undocumented attributes:
     distance: Optional[float] = None
     start_date: Optional[datetime] = None
@@ -734,21 +739,16 @@ class AthletePrEffort(
 
     _naive_local = field_validator("start_date_local")(naive_datetime)
 
-    @property
-    def elapsed_time(self) -> Optional[int]:
-        """A property that supports backwards compatibility with the
-        elapsed_time method used in previous versions of stravalib"""
-
-        return self.pr_elapsed_time
-
 
 class SummarySegment(strava_model.SummarySegment, BoundClientEntity):
     # Field overrides from superclass for type extensions:
     start_latlng: Optional[LatLon] = None
     end_latlng: Optional[LatLon] = None
-    athlete_segment_stats: Optional[AthleteSegmentStats] = None
     athlete_pr_effort: Optional[AthletePrEffort] = None
     activity_type: Optional[RelaxedActivityType] = None  # type: ignore[assignment]
+    athlete_segment_stats: Optional[AthleteSegmentStats] = (
+        None  # Actually, this is only part of a (detailed) segment response
+    )
 
     _latlng_check = field_validator(
         "start_latlng", "end_latlng", mode="before"
@@ -801,8 +801,23 @@ class SegmentEffortAchievement(BaseModel):
     effort_count: Optional[int] = None
 
 
+class SummarySegmentEffort(strava_model.SummarySegmentEffort):
+    # Override superclass fields to match actual Strava API responses
+    activity_id: Optional[int] = Field(
+        validation_alias=AliasChoices("pr_activity_id", "activity_id"),
+        default=None,
+    )
+    elapsed_time: Optional[int] = Field(
+        validation_alias=AliasChoices("pr_elapsed_time", "elapsed_time"),
+        default=None,
+    )
+
+    _naive_local = field_validator("start_date_local")(naive_datetime)
+
+
 class BaseEffort(
-    DetailedSegmentEffort,
+    SummarySegmentEffort,
+    strava_model.DetailedSegmentEffort,
     BoundClientEntity,
 ):
     """
@@ -811,10 +826,8 @@ class BaseEffort(
 
     # Field overrides from superclass for type extensions:
     segment: Optional[SummarySegment] = None
-    activity: Optional[Activity] = None
-    athlete: Optional[Athlete] = None
-
-    _naive_local = field_validator("start_date_local")(naive_datetime)
+    activity: Optional[MetaActivity] = None
+    athlete: Optional[MetaAthlete] = None
 
 
 class BestEffort(BaseEffort):
@@ -833,6 +846,18 @@ class SegmentEffort(BaseEffort):
     achievements: Optional[list[SegmentEffortAchievement]] = None
 
 
+class AthleteSegmentStats(
+    SummarySegmentEffort,
+):
+    """
+    A structure being returned for segment stats for current athlete.
+    """
+
+    # Undocumented attributes:
+    effort_count: Optional[int] = None
+    pr_date: Optional[date] = None
+
+
 class MetaActivity(strava_model.MetaActivity, BoundClientEntity):
     @lazy_property
     def comments(self) -> BatchedResultsIterator[ActivityComment]:
@@ -871,14 +896,17 @@ class MetaActivity(strava_model.MetaActivity, BoundClientEntity):
 class SummaryActivity(MetaActivity, strava_model.SummaryActivity):
     # field overrides from superclass for type extensions:
     athlete: Optional[MetaAthlete] = None
-    start_latlng: Optional[LatLon] = (
-        None  # Do we still need these overrides for latlon?
-    )
+    # These force validator to run on lat/lon
+    start_latlng: Optional[LatLon] = None
     end_latlng: Optional[LatLon] = None
     map: Optional[Map] = None
     type: Optional[RelaxedActivityType] = None
     sport_type: Optional[RelaxedSportType] = None
 
+    _latlng_check = field_validator(
+        "start_latlng", "end_latlng", mode="before"
+    )(check_valid_location)
+
 
 class Activity(
     SummaryActivity,
@@ -930,9 +958,6 @@ class Activity(
     segment_leaderboard_opt_out: Optional[bool] = None
     perceived_exertion: Optional[int] = None
 
-    _latlng_check = field_validator(
-        "start_latlng", "end_latlng", mode="before"
-    )(check_valid_location)
     _naive_local = field_validator("start_date_local")(naive_datetime)
 
 
@@ -1063,8 +1088,3 @@ class SubscriptionUpdate(BaseModel):
     aspect_type: Optional[str] = None
     event_time: Optional[datetime] = None
     updates: Optional[dict[str, Any]] = None
-
-
-SegmentEffort.update_forward_refs()
-ActivityLap.update_forward_refs()
-BestEffort.update_forward_refs()


Test patch:
diff --git a/src/stravalib/tests/integration/test_client.py b/src/stravalib/tests/integration/test_client.py
index c2ee3b0..9ae0675 100644
--- a/src/stravalib/tests/integration/test_client.py
+++ b/src/stravalib/tests/integration/test_client.py
@@ -733,14 +733,19 @@ def test_get_club(mock_strava_api, client):
 
 
 @pytest.mark.parametrize("n_clubs", (0, 2))
-def test_get_athlete_clubs(mock_strava_api, client, n_clubs):
+def test_get_athlete_clubs_iterator(mock_strava_api, client, n_clubs):
+    """Test that athlete clubs returns the correct
+    number of clubs"""
+
+    # Create a mock instance with the club name update to my club
     mock_strava_api.get(
-        "/athlete/clubs", response_update={"name": "foo"}, n_results=n_clubs
+        "/athlete/clubs", response_update={"name": "myclub"}, n_results=n_clubs
     )
-    clubs = client.get_athlete_clubs()
+    # Convert iterator to list
+    clubs = list(client.get_athlete_clubs())
     assert len(clubs) == n_clubs
     if clubs:
-        assert clubs[0].name == "foo"
+        assert clubs[0].name == "myclub"
 
 
 @pytest.mark.parametrize("n_members", (0, 2))
diff --git a/src/stravalib/tests/unit/test_model.py b/src/stravalib/tests/unit/test_model.py
index 3fdd6dd..d3331c5 100644
--- a/src/stravalib/tests/unit/test_model.py
+++ b/src/stravalib/tests/unit/test_model.py
@@ -18,11 +18,12 @@ from stravalib.model import (
     RelaxedSportType,
     Route,
     Segment,
+    SegmentEffort,
     SegmentExplorerResult,
     Split,
     SubscriptionCallback,
+    SummarySegmentEffort,
 )
-from stravalib.strava_model import LatLng
 from stravalib.tests import TestBase
 
 
@@ -30,16 +31,31 @@ from stravalib.tests import TestBase
     "model_class,raw,expected_value",
     (
         (
-            # Removing root
+            Activity,
+            {"start_latlng": []},
+            None,
+        ),
+        (
+            Activity,
+            {"end_latlng": []},
+            None,
+        ),
+        (
+            Activity,
+            {"end_latlng": "5.4,4.3"},
+            LatLon([5.4, 4.3]),
+        ),
+        (
             Activity,
             {"start_latlng": "5.4,4.3"},
             LatLon([5.4, 4.3]),
-        ),  # pydantic 2.x uses root and may
+        ),
         (Activity, {"start_latlng": []}, None),
         (Segment, {"start_latlng": []}, None),
         (SegmentExplorerResult, {"start_latlng": []}, None),
         (ActivityPhoto, {"location": []}, None),
-        # (Activity, {"timezone": "foobar"}, None),  TODO re-add this test when custom types are implemented
+        # TODO re-add this Activity test when custom types are implemented
+        # (Activity, {"timezone": "foobar"}, None),
         (
             Activity,
             {"start_date_local": "2023-01-17T11:06:07Z"},
@@ -55,6 +71,16 @@ from stravalib.tests import TestBase
             {"start_date_local": "2023-01-17T11:06:07Z"},
             datetime(2023, 1, 17, 11, 6, 7),
         ),
+        (
+            SummarySegmentEffort,
+            {"start_date_local": "2023-01-17T11:06:07Z"},
+            datetime(2023, 1, 17, 11, 6, 7),
+        ),
+        (
+            SegmentEffort,
+            {"start_date_local": "2023-01-17T11:06:07Z"},
+            datetime(2023, 1, 17, 11, 6, 7),
+        ),
     ),
 )
 def test_deserialization_edge_cases(model_class, raw, expected_value):
@@ -62,6 +88,46 @@ def test_deserialization_edge_cases(model_class, raw, expected_value):
     assert getattr(obj, list(raw.keys())[0]) == expected_value
 
 
+@pytest.mark.parametrize(
+    "model_class,raw,parsed_attr,expected_parsed_attr_value",
+    (
+        (SummarySegmentEffort, {"pr_activity_id": 42}, "activity_id", 42),
+        (SummarySegmentEffort, {"activity_id": 42}, "activity_id", 42),
+        (
+            SummarySegmentEffort,
+            {"pr_elapsed_time": 42},
+            "elapsed_time",
+            42,
+        ),
+        (
+            SummarySegmentEffort,
+            {"elapsed_time": 42},
+            "elapsed_time",
+            42,
+        ),
+        (AthletePrEffort, {"pr_activity_id": 42}, "activity_id", 42),
+        (AthletePrEffort, {"activity_id": 42}, "activity_id", 42),
+        (
+            AthletePrEffort,
+            {"pr_elapsed_time": 42},
+            "elapsed_time",
+            42,
+        ),
+        (
+            AthletePrEffort,
+            {"elapsed_time": 42},
+            "elapsed_time",
+            42,
+        ),
+    ),
+)
+def test_strava_api_field_name_inconsistencies(
+    model_class, raw, parsed_attr, expected_parsed_attr_value
+):
+    obj = model_class.model_validate(raw)
+    assert getattr(obj, parsed_attr) == expected_parsed_attr_value
+
+
 def test_subscription_callback_field_names():
     sub_callback_raw = {
         "hub.mode": "subscribe",
@@ -73,11 +139,6 @@ def test_subscription_callback_field_names():
     assert sub_callback.hub_verify_token == "STRAVA"
 
 
-class ExtLatLng(LatLng):
-    def foo(self):
-        return f"[{self[0], self[1]}]"
-
-
 # TODO: do we want to continue to support type?
 @pytest.mark.parametrize(
     "klass,attr,given_type,expected_type",