-
-
Notifications
You must be signed in to change notification settings - Fork 196
Add Type Hints to caselessdict.py #1015
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
Conversation
Pull Request Test Coverage Report for Build 19876975935Details
💛 - Coveralls |
stevepiercy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions fix minor docs issues and improve a few others.
This needs a technical review, but it looks good.
SashankBhamidi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be merged after Steve's suggestions + Optional[Any] fix applied
Co-authored-by: Steve Piercy <[email protected]>
Co-authored-by: Steve Piercy <[email protected]>
Co-authored-by: Steve Piercy <[email protected]>
niccokunzmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution and the tests.... ! It looks good generally. I left some comments. What are your thoughts?
Co-authored-by: Nicco Kunzmann <[email protected]>
Co-authored-by: Nicco Kunzmann <[email protected]>
Co-authored-by: Nicco Kunzmann <[email protected]>
|
Thanks, these suggestions all make sense! Once this PR is ready to be merged, I’d love to help with adding type hints to other modules as well. Are there any files you’d recommend I look at next, or should I start exploring and find untyped areas myself? |
|
@lillianpenner It looks like tests are passing. I was wondering just now: Are keys always str in a CaselessDict? I think so... That could be changed. What do you think? When you looked through it does anything else than str as a key make sense? When I think about what would be nice next... Probably looking at the public API helps to find the most impactful places. E.g. This is a valid contribution and can be merged from my perspective. |
|
@niccokunzmann This brings up a good point. Here’s how I’m thinking about it: Because of that, I think parameter types for key should stay as Any, since callers may pass non-string keys, while the stored and returned keys are always str, since normalization happens before insertion and lookup. Please let me know if I’m misunderstanding anything here! |
No. @lillianpenner @niccokunzmann for icalendar, though, I can't imagine using anything beside strings or bytes for a key in So I'd make sure that each key is either a string or bytes and nothing else in its Finally, I don't know if it's possible to enforce this through typing, but |
stevepiercy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs LGTM.
See my last comment and let's discuss whether anything further should be changed.
|
@stevepiercy Thanks for the explanation and the context around CaselessDict keys. That makes sense. For this PR, I avoided changing any runtime behavior, which is why I left the key parameter typed as Any. I wasn’t sure whether adding runtime restrictions in init would introduce compatibility issues, but I agree that tightening this area could be valuable. Would you prefer that I explore these ideas in this PR or follow up with these separately? Side note: I didn't mean to close the PR, but I just reopened it, oops |
|
@lillianpenner this PR is an improvement because it adds typing. On that point alone, I'd say that this is OK to merge on the condition that a new issue be used specifically for refining the typing, and possibly the code, to enforce that the keys in a @niccokunzmann @SashankBhamidi thoughts? |
|
Agree, this PR improves typing without breaking changes. The broader key type enforcement discussion should continue in #1016 as Steve suggested. |
SashankBhamidi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving to merge.
|
Yes, thanks, way better than before and we can sort our ideas out in another issue. |
Closes issue
Description
This pull request adds type hints to CaselessDict and to the helper functions canonsort_keys and canonsort_items. These annotations improve static analysis and readability while preserving existing behavior. No runtime logic was modified.
Checklist
CHANGES.rst.docs/credits.rstas a contributor in this pull request or have done so previously.Additional information
Tests were run in a fresh virtual environment using:
tox
Results:
= 9698 passed, 21 skipped in 24.68s
I added a small test file (
test_type_hints.py) to exerciseCaselessDict,canonsort_keys, andcanonsort_items, and confirm existing behavior with type hints.📚 Documentation preview 📚: https://icalendar--1015.org.readthedocs.build/