BUG: Mypy types possibly incorrect

[JohnScolaro]

Stravalib version checks

• [X] I have tested this in a new clean environment with only stravalib and core python files.

• [X] I have checked that this issue has not already been reported.

• [X] I have confirmed this bug exists on the latest version of stravalib.

• [X] I have confirmed this bug exists on the main branch of stravalib.

What operating system are you seeing the problem on?

Windows

What version of python or you running?

3.10.12

Reproducible Example

from stravalib.client import Client

client = Client()
authorize_url = client.authorization_url(
    client_id=YOUR_CLIENT_ID, redirect_uri="https://active-statistics.com"
)

print(authorize_url)

token = input()

token_response = client.exchange_code_for_token(
    client_id=YOUR_CLIENT_ID,
    client_secret=YOUR_CLIENT_SECRET,
    code=token,
)
access_token = token_response["access_token"]
refresh_token = token_response["refresh_token"]
expires_at = token_response["expires_at"]

# Now store that short-lived access token somewhere (a database?)
client.access_token = access_token

activity = client.get_activity(ACTIVITY_ID, include_all_efforts=True)

elapsed_time = activity.elapsed_time
if elapsed_time is not None:
    elapsed_time_in_seconds = elapsed_time.total_seconds()

print(elapsed_time_in_seconds)

Issue Description

Hey guys, I'm not sure if this is an issue or not but:

Because of how the _field_conversions work in Stravalib, I can have perfectly valid code like in the example I pasted above, and mypy fails. Above, I query an activity, then get the elapsed time in seconds from it. The Activity object is typed to hold the elapsed_time attribute as an integer. However, because (I'm not sure the exact process here, but) _field_conversions is turning it into a timedelta when accessed, when I run mypy I get the following errors:

test.py:32: error: "int" has no attribute "total_seconds"  [attr-defined]

Am I missing something here, or should the Activity types somehow be changed so I don't get hundreds of mypy errors from working code?

Expected Behavior

I expect mypy to pass when working with Activities in a valid manor.

Your environment

arrow==1.2.3 certifi==2023.7.22 charset-normalizer==3.2.0 idna==3.4 mypy==1.5.1 mypy-extensions==1.0.0 Pint==0.22 pydantic==1.10.9 python-dateutil==2.8.2 pytz==2023.3.post1 requests==2.31.0 six==1.16.0 stravalib==1.5 tomli==2.0.1 typing_extensions==4.7.1 urllib3==2.0.4

Code of Conduct

• [X] I agree to follow this project's Code of Conduct

[enadeau]

Hey, thanks for reporting the bug.

Your are rigth that it is connected to the _field_conversion. Those are conversion function for specific field that the BackwardCompatibilityMixin class take care of applying when accessing attributes.

I removed the elapsed time from the _field_conversion on Activity and instead redefined it as field with type Optional[timedelta] on Activity. That fixed the typing error in the example above.

@@ -1087,7 +1087,6 @@ class Activity(
 
     _field_conversions = {
         "moving_time": time_interval,
-        "elapsed_time": time_interval,
         "timezone": timezone,
         "distance": uh.meters,
         "total_elevation_gain": uh.meters,
@@ -1096,6 +1095,7 @@ class Activity(
         "type": enum_value,
         "sport_type": enum_value,
     }
+    elapsed_time: Optional[timedelta] = None # type: ignore[assignment]
 
     _latlng_check = validator(
         "start_latlng", "end_latlng", allow_reuse=True, pre=True

From what I can see BackwardCompatibilityMixin only function is to apply those conversions. My suggested plan goes lie this.

• Remove BackwardCompatibilityMixin
• For each item that goes through a field conversion, instead define a field with the right type on our model (we will for some need to use pydantic valdiator to actually perform the conversion)
• Remove the _field_conversion attribute from all classes

@jsamoocha and @lwasser any thoughts on that?

[jsamoocha]

BackwardCompatibilityMixin's attribute lookup does something else: it passes the bound client to an object's child members if these are of type BoundClientEntity. If we remove that, it'll create breaking changes.

I agree that eventually, we'll need to get rid of BackwardCompatibilityMixin (and DeprecatedSerializableMixin) to simplify the code. But in the short term, maybe the first step could be to see if we could replace these _field_conversions by Pydantic validators as you suggested.

I hope I have a bit more time to look into this (and upgrading to Pydantic2 and looking at that failing daily job that's supposed to update the model) in a few weeks.

[lwasser]

hey y'all - i can have a look at the failing build later today. it's weirdly struggling to find the swagger.json file.

I also noticed a few other builds failed for odd reasons. i'll create a new issue for that specific problem.

[enadeau]

Okay missed the part about bound client I now see where it is hidden in the function.

I'm going to try to replace all those field conversion by validator and open a PR.

[enadeau]

#440 implements a fix for the case where the field are timedelta. The other case are not valid pydantic type we would need to allow aribtraty type on the model. See https://docs.pydantic.dev/1.10/usage/types/#arbitrary-types-allowed

If you think this is the way to go I could try to make PR with that

[lwasser]

hey there everyone. i'm following up on this issue (in an attempt to clean things up)

it looks like we already implemented a change to allow for timedelta to be a valid return type . I'm trying to understand the scenario where a type other than int (seconds), timedelta would be returned that would require an arbitrary type / composition type approach.

do we need to investigate this further? it does seem like we may want to start working on the pydantic 2.0 migration and clean up all of the legacy support (which will also simplifuy the code as @jsamoocha points out above. so my quesiton is, should we spend more time on this issue OR should we begin effort towards migration?

i have been using pydantic 2 and can help.

[enadeau]

If I remember correctly there were also issues with everything that uses the pint library.

That is something I'd like to discuss moving to pydantic v2 + stravalib v2 as I not sure I see the value in it. I should probably open an other issue or find a better place to discuss the breaking changes we want to do.

[lwasser]

@enadeau that would be great! please go ahead and open another issue! i don't remember the issues with pint. I think a while back we had discussed creating new issue where we try to break out what need to be implemented in the migration. If we did that, we could then create a stravalibv2 branch here, i could create a milestone with issues needed to be implemented in the migration and we all could (as time allows) start submitting pr's to that branch as a way to begin the migration without any impact to the current "production" code base. Just a thought!

i have travel coming up but also there are always days like this when i'm home and the weather is not great and i can chip away at things!

[jsamoocha]

Let's try to incorporate this in the v2 change.

[lwasser]

Great i've moved this to the pydantic 2.x milestone

[lwasser]

all - i'm checking in again! i think we now have dealt with this via a few pr's linked above #498 are there any missing elements to be aware of or can we close this issue (please note that to test the fix, you'd need to using our pydantic-v2 branch. 🙌🏻