-
Notifications
You must be signed in to change notification settings - Fork 202
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
BUG: Mypy types possibly incorrect #437
Comments
Hey, thanks for reporting the bug. Your are rigth that it is connected to the I removed the elapsed time from the @@ -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
@jsamoocha and @lwasser any thoughts on that? |
I agree that eventually, we'll need to get rid of 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. |
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. |
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. |
#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 |
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? i have been using pydantic 2 and can help. |
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. |
@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! |
Let's try to incorporate this in the v2 change. |
Great i've moved this to the pydantic 2.x milestone |
Stravalib version checks
I have tested this in a new clean environment with only stravalib and core python files.
I have checked that this issue has not already been reported.
I have confirmed this bug exists on the latest version of stravalib.
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?
Reproducible Example
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. TheActivity
object is typed to hold theelapsed_time
attribute as an integer. However, because (I'm not sure the exact process here, but)_field_conversions
is turning it into atimedelta
when accessed, when I run mypy I get the following errors: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
The text was updated successfully, but these errors were encountered: