Leverage type hints

[aahnik]

fixes #6

• Make Mypy run in the CI
• Bundle a py.typed file
• Add type hints wherever possible
• satisfy mypy fully

[codecov-commenter]

Codecov Report

Merging #21 (4b3a207) into main (04451fc) will increase coverage by 1.25%.
The diff coverage is 77.94%.

@@            Coverage Diff             @@
##             main      #21      +/-   ##
==========================================
+ Coverage   69.27%   70.52%   +1.25%     
==========================================
  Files           6        6              
  Lines         179      190      +11     
==========================================
+ Hits          124      134      +10     
- Misses         55       56       +1     

Impacted Files	Coverage Δ
notion_client/client.py	65.43% <65.85%> (-8.34%)	⬇️
notion_client/api_endpoints.py	69.23% <94.11%> (+0.80%)	⬆️
notion_client/__init__.py	100.00% <100.00%> (ø)
notion_client/errors.py	72.72% <100.00%> (+10.96%)	⬆️
notion_client/helpers.py	66.66% <100.00%> (+16.66%)	⬆️
notion_client/logging.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04451fc...4b3a207. Read the comment docs.

[ramnes]

Have a look at this comment on Reddit. The first point they make about the current classes inheritance might bite you.

[aahnik]

I see AsyncClient inherits from Client and overrides its request method. Having recently worked on building a sync/async client, I can tell you that while this works, you'll never be able to make mypy happy, and it kind of breaks the Liskov substitution principle.

What I'd recommend instead is that there's a BaseClient abstract class which concerns itself with all non-transport logic, and that Client and AsyncClient then subclass the base client (but not each other) and implement their transport methods, using the base class' methods for validating/parsing data, etc.

this idea by @pedrovhb is great. I think we should do that.

this would fix this

notion_client/client.py:93: error: Argument 2 to "init" of "Client" has incompatible type "AsyncClient"; expected "Client"

the base client should accept both AsyncClient, as well as Client

[aahnik]

hi @ramnes, I am a little less confident about ff9e260.

please review to make sure I have made no mistakes.

As explicit is better than implicit, I have added -> None to functions that return None.

[ramnes]

LGTM so far overall but there are still a lot of Mypy errors to fix!

structural changes need to made in the code. I am afraid of doing that, as it might lead to conflicts with other open PRs.

Don't be afraid, one of the two PR is only working in examples/, and the other one is a big WIP that will have to live with rebases anyway, and I'll give some help if needed. :)

Do you mind if I add commits to this branch to give a hand?

[aahnik]

Do you mind if I add commits to this branch to give a hand?

That will be great! Please do so.

[aahnik]

Thanks for the commits. I was doing lot of things wrong, as I was driven by the urge to suppress mypy. I had a few unpushed commits, but I deleted them,as they were dangerous.(huge code repition)

I have no idea how to fix, the remaining mypy errors. 😢 Its all up to you now.🙌

[ramnes]

Yeah I can take over, no worries, thanks a lot for the work so far!

[aahnik]

How did mypy pass in CI ?

Running mypy locally still gives two errors:

➜ mypy notion_client
notion_client/client.py:128: error: Argument 1 to "_check_response" of "BaseClient" has incompatible type "Union[Response, Coroutine[Any, Any, Response]]"; expected "Response"
notion_client/client.py:129: error: Incompatible return value type (got "Union[Response, Coroutine[Any, Any, Response]]", expected "Response")
Found 2 errors in 1 file (checked 6 source files)

[ramnes]

Yeah, I wondered that as well. Answer here. :)

[ramnes]

Let's go! 🎉

Thank you again for your very motivating efforts @aahnik.