Begin type hinting

[enadeau]

Description

Introduce type annotation and static type checking as discussed in #373

This include some trivial changes to some function.

The only change that I can see breaking something for user is the removal of the function
total_seconds from the limiter file that seems to be some workaround for
python pre 2.7.

Type of change

Select the statement best describes this pull request.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update
• This is a documentation update
• Other (please describe)

Does your PR include tests

If you are fixing a bug or adding a feature, we appreciate (but do not require)
tests to support whatever fix of feature you're implementing.

• Yes
• No, i'd like some help with tests
• This change doesn't require tests

Did you include your contribution to the change log?

• Yes, changelog.md is up-to-date.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@20afb17). Click here to learn what that means.
The diff coverage is 85.18%.

@@            Coverage Diff            @@
##             master     #374   +/-   ##
=========================================
  Coverage          ?   85.89%           
=========================================
  Files             ?       10           
  Lines             ?     1666           
  Branches          ?      157           
=========================================
  Hits              ?     1431           
  Misses            ?      192           
  Partials          ?       43           

Impacted Files	Coverage Δ
stravalib/protocol.py	65.87% <67.74%> (ø)
stravalib/util/limiter.py	77.69% <96.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[jsamoocha]

Hi, thanks a lot for these contributions so far. Should we see this PR as WIP and wait to review it until you indicate you're finished?

BTW, I don't understand how your latest commit with the modern typing syntax (e.g., foo: int | None instead of foo: Optional[int]) runs successfully on py3.8 (as can be seen by the test results in CI). Do you have any idea?

[enadeau]

Hi yes best to see it as WIP. I changed it to draft status. I'm working on typing the client now and as I go I sometime go back and tweak those two file. I'll let you know when I'm confident enough to send it for review.

The changes work because of the from __future__ import annotations statement which caused all the type annotation to be converted to string and therefore not evaluated at runtime. Mypy and other tool however still understand it.

[enadeau]

I'm quite confident the type are correct now but worst case scenario we can tweak them in future PR. Ready for review

[lwasser]

@enadeau wow - you are contributing to much to this package!! would you mind adding 2 entries to our changelog that includes this PR and the other just merged?

it would look something like this:

# Change Log

## Unreleased
* Single line description of the fix in the pr (@enadeau , #issue-number-here) 
* Another single line description of the fix in the pr (@enadeau , #issue-number-here) 

If there is no related issue you can simply just list the pr number :) we just want to ensure you get credit for these awesome contributions!!

[lwasser]

ignore the above. i needed to update pydantic to 1.9+

[lwasser]

this is great that we can limit what mypy looks at. what i wasn't sure about is whether our pre-commit.ci bot will follow this configuration (it might). it seems to want to run on all files which is what i was worried about with CI. We could test that on this PR if you wish to add the config to the pre-commit.config.yaml file.

[enadeau]

I'm very happy to contribute 😄

I've updated the changelog.

About mypy, I've added a workflow to run it instead of putting it in pre-commit. Because pre-commit is designed to run on diff file and mypy really want to analyze the whole program, it is generally not recommended to have mypy as a pre-commit hook. All repos I've worked with run it as an action. Let me know what you think

[yihong0618]

LGTM
nice work, thanks a lot.

[lwasser]

@enadeau many thanks for this!

About mypy, I've added a workflow to run it instead of putting it in pre-commit. Because pre-commit is designed to run on diff file and mypy really want to analyze the whole program, it is generally not recommended to have mypy as a pre-commit hook. All repos I've worked with run it as an action. Let me know what you think

I think that you know a lot more about implementing mypy that I do! Thus, I fully trust your evaluation and suggestion that we add it as a ci build. I am sorry i pushed back against that earlier!! I should have asked you about things like - specifying specific files for it to run on as we get other pr's!

[lwasser]

I think we can approve this PR. i have requested just a tiny change to the name of the type checking file just so it's clear what that action does. but i'm approving this PR as it is a great addition to this package. @jsamoocha would love to get your input before merging in case i'm missing something!!

[enadeau]

I changed the name and for what I'm concerned it is ready to go. Not quite sure what's up with the documentation build. Seems to complain a lot about findfont: Font family 'Roboto' not found. Can someone help me with that?

[lwasser]

oh that is an unusual error. Let me dig into it.

[lwasser]

I reran the job and it built just fine. That warning is still popping up 🤷‍♀️ . I think we can merge this PR without worrying about it because i suspect it's a theme issue not a stravalib issue. It's certainly not related to anything that you have implemented in this pr!! i'll keep an eye on the docs build for future pr's.

We can open an issue if it pops up again OR if we find the readthedocs is not building as it should.

also many thanks for modifying that file name!

[lwasser]

We have two approvals here and all checks are passing so i'll go ahead and merge!! 🚀

[jsamoocha]

Thanks again @enadeau, adding mypy to CI is a great addition to improve our code quality. Awesome contribution!