Skip to content
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

Add inline typing #358

Merged
merged 1 commit into from
Dec 18, 2023
Merged

Add inline typing #358

merged 1 commit into from
Dec 18, 2023

Conversation

sim0nx
Copy link
Contributor

@sim0nx sim0nx commented Dec 18, 2023

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:

  • testSearchEvent
  • testTodoDatesearch
  • testInviteAndRespond

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
Copy link
Member

tobixen commented Dec 18, 2023

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
Copy link
Contributor Author

sim0nx commented Dec 18, 2023

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
Copy link
Member

tobixen commented Dec 18, 2023

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
Copy link
Contributor Author

sim0nx commented Dec 18, 2023

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
Copy link
Contributor Author

sim0nx commented Dec 18, 2023

There you go, 1 commit :-)

@tobixen tobixen added this pull request to the merge queue Dec 18, 2023
Merged via the queue into python-caldav:master with commit 722e9c0 Dec 18, 2023
7 checks passed
@sim0nx sim0nx deleted the inline_typing branch December 18, 2023 20:28
@sim0nx sim0nx mentioned this pull request Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typehints should be implemented
2 participants