python-caldav / caldav

Apache License 2.0
313 stars 91 forks source link

Add inline typing #358

Closed sim0nx closed 7 months ago

sim0nx commented 7 months ago

This fixes #328 by inlining type hints from typeshed caldav. Types were later fixed and further implemented where missing.

Tests all pass using Xandikos. davical was also tested, here 3 tests fail which also fail with the current master:

I tried as much as possible not to touch any actual code, besides adding "None" checks. No logic was changed.

Types were verified using typeguard (pytest extension). Also pre-commit hooks were run.

Could you please have a look and let me know if there is anything left to do for this to get merged ?

tobixen commented 7 months ago

Indeed, I've noticed myself that three tests now are breaking towards DAViCal, I will raise a separate issue on that.

Now it may seem like you've forgotten to add a typehints file? Or perhaps it's a library dependency that needs to be added?

I'd appreciate it if you could rebase it into one commit. I'd also appreciate it if you would update the CHANGELOG.md (the latter is the least important - I usually take care of that myself).

Thanks a lot for your efforts and contribution!

sim0nx commented 7 months ago

Now it may seem like you've forgotten to add a typehints file? Or perhaps it's a library dependency that needs to be added?

Indeed... I have fixed that (typing_extensions). I ran the tests now on 3.7 and 3.8 as well and they work. Though I would suggest dropping 3.7 support (which is EoL anyways).

I'd appreciate it if you could rebase it into one commit. I'd also appreciate it if you would update the CHANGELOG.md (the latter is the least important - I usually take care of that myself). Thanks a lot for your efforts and contribution!

I can do that if you prefer, though an alternative would be to squash+merge here in github ? That does that. Let me know if you still want me to do it.

I'll add a changelog entry.

tobixen commented 7 months ago

Indeed... I have fixed that (typing_extensions). I ran the tests now on 3.7 and 3.8 as well and they work. Though I would suggest dropping 3.7 support (which is EoL anyways).

Well, there are still LTS-distros out there offering python2 if I'm not mistaken. I prefer to be conservative when it comes to support for old versions.

I can do that if you prefer, though an alternative would be to squash+merge here in github ? That does that. Let me know if you still want me to do it.

I was playing a bit with a "merge queue"-feature at github, after that the "squash+merge"-button seems to have disappeared - perhaps I should look into the github project settings again and disable the "merge queue"-thing (I understood it so that it would wait until tests have run and then merge things only if all tests pass, which could be useful, but somehow it just waits until all tests have run and then it merges things even if there are failing tests). I could of course also do a git rebase at my side - but then probably git would set me to the committer. I think it's preferable that you do the rebase to get the history correct.

sim0nx commented 7 months ago

Indeed... I have fixed that (typing_extensions). I ran the tests now on 3.7 and 3.8 as well and they work. Though I would suggest dropping 3.7 support (which is EoL anyways).

Well, there are still LTS-distros out there offering python2 if I'm not mistaken. I prefer to be conservative when it comes to support for old versions.

ok

I can do that if you prefer, though an alternative would be to squash+merge here in github ? That does that. Let me know if you still want me to do it.

I was playing a bit with a "merge queue"-feature at github, after that the "squash+merge"-button seems to have disappeared - perhaps I should look into the github project settings again and disable the "merge queue"-thing (I understood it so that it would wait until tests have run and then merge things only if all tests pass, which could be useful, but somehow it just waits until all tests have run and then it merges things even if there are failing tests). I could of course also do a git rebase at my side - but then probably git would set me to the committer. I think it's preferable that you do the rebase to get the history correct.

IIRC the same will happen when you merge it in github. anyways I will see how I can squash it and push it 🙂

sim0nx commented 7 months ago

There you go, 1 commit :-)