8000 Implemented typehints for value parameter of IntervalDict by fxf8 · Pull Request #99 · AlexandreDecan/portion · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Implemented typehints for value parameter of IntervalDict #99

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

Merged
merged 24 commits into from
Mar 27, 2025

Conversation

fxf8
Copy link
@fxf8 fxf8 commented Mar 24, 2025

It is a big change the IntervalDict implementation. However, I believe this is useful to annotate the values even though the keys are somewhat unannotatable. For the keys however, I annotated them with object with the intent that users will use the correct type.

@AlexandreDecan
Copy link
Owner

Hi!

Are there cases where a static type checker is helpful with those annotations? I mean, having object (or Any) is not really helpful for type checking and "keys" are always instances of Interval.

@fxf8
Copy link
Author
fxf8 commented Mar 24, 2025

Hi!

Are there cases where a static type checker is helpful with those annotations? I mean, having object (or Any) is not really helpful for type checking and "keys" are always instances of Interval.

Yes, there is a use for annotating keys with object. Firstly, it ensures the user that none of its methods will be called (as opposed to Any where any methods can be called). Secondly, it allows you to differentiate between Interval. In methods overloaded with Interval and object as keys, it is then possible to specify output types for Interval and object

@AlexandreDecan
Copy link
Owner

Oh, thanks, I wasn't aware of this difference between "object" and "Any".

Considering that only this part of portion has types at the moment, would you consider moving this typing to a stub file?

@fxf8
Copy link
8000 Author
fxf8 commented Mar 24, 2025

Oh, thanks, I wasn't aware of this difference between "object" and "Any".

Considering that only this part of portion has types at the moment, would you consider moving this typing to a stub file?

Sure. I have moved the typings to a typestub file

Edit: Nevermind, theres something I have to fix

@fxf8
Copy link
Author
fxf8 commented Mar 24, 2025

Oh, thanks, I wasn't aware of this difference between "object" and "Any".

Considering that only this part of portion has types at the moment, would you consider moving this typing to a stub file?

Everything should be good now. I have added the type stub files and tested the library in my editor.

@AlexandreDecan
Copy link
Owner

Thanks, I'll have a look at it soon.

Are there ways to check automatically the consistency of a stub file with its code? So that, if a change parts of portion, I can be warned by our CI that I need to keep the stub up to date?

@AlexandreDecan
Copy link
Owner

Everything should be good now. I have added the type stub files and tested the library in my editor.

By "stub file", I meant avoiding type annotations within portion's code and to have them exclusively in a stub file :-)
I'll integrate type annotations to portion's code the day we'll cover everything in an elegant way (something that is unfortunately not doable currently, at least for Interval).

@AlexandreDecan AlexandreDecan added the enhancement New feature or request label Mar 25, 2025
@AlexandreDecan AlexandreDecan linked an issue Mar 25, 2025 that may be closed by this pull request
@AlexandreDecan AlexandreDecan removed a link to an issue Mar 25, 2025
@fxf8
Copy link
Author
fxf8 commented Mar 25, 2025

Everything should be good now. I have added the type stub files and tested the library in my editor.

By "stub file", I meant avoiding type annotations within portion's code and to have them exclusively in a stub file :-) I'll integrate type annotations to portion's code the day we'll cover everything in an elegant way (something that is unfortunately not doable currently, at least for Interval).

Okay! Sorry, I had some trouble earlier with where to place the stub files and their configurations in pyproject.toml, but I have tested them and I believe they work properly now. I have reverted dict.py to the original and all typings are in portion/dict.pyi

@AlexandreDecan
Copy link
Owner

Thanks! I'm not experienced with stub files (nor with type hinting in general, I did it only once for another, larger library). Is there a need for https://github.com/AlexandreDecan/portion/blob/2f2c2e6a289bdb9efe104420b8a13b48ed23d07e/portion/__init__.pyi or can we simply provide dict.pyi ?

Other related questions:

  • Have you any advice for integrating type checking/stub file consistency as part of our GHA workflows? I quickly Googled and found that stubtest could be helpful for consistency checks... Any opinion on this?
  • Have you any plan yet for other parts of portion?
  • Is it better to distribute (partial) stub files as part of portion or to distribute them on typeshed (https://github.com/python/typeshed)?

Thanks!

@fxf8
Copy link
Author
fxf8 commented Mar 25, 2025

Thanks! I'm not experienced with stub files (nor with type hinting in general, I did it only once for another, larger library). Is there a need for 2f2c2e6/portion/init.pyi or can we simply provide dict.pyi ?

Other related questions:

* Have you any advice for integrating type checking/stub file consistency as part of our GHA workflows? I quickly Googled and found that `stubtest` could be helpful for consistency checks... Any opinion on this?

* Have you any plan yet for other parts of `portion`?

* Is it better to distribute (partial) stub files as part of `portion` or to distribute them on typeshed ([python/typeshed](https://github.com/python/typeshed?rgh-link-date=2025-03-25T14%3A21%3A51Z))?

Thanks!

I think you're right, we can remove __init__.py

I am also not an expert on the tooling surrounding typehints, so I don't have experience-based advice on typestub checking. I have tried using stubtest, and it reminded me of certain conflicts: The actual class IntervalDict technically should not derive from MutableMapping because the call signature of get, pop, and update is not compatible. But realistically, the library functions perfectly fine.

However, I would like to point out the intent of type hints in the first place. Within your docstrings inside the methods, you have notes such as "Given X type, this returns Y" which is essentially part of what the typehints do, they are just like comments. I think it's reasonable to say that the functionality does not decide the typehints, but that the typehints are describe what the functionality does. From my experience, typehints are part of the planning stage. But in this case, IntervalDict already works perfectly, so I don't really know how we would check them. Aside from the overriding incompatibility with MutableMapping, running stubtest portion does not seem to raise any other issues which makes sense since part of my annotations consisted of resolving all substantial type errors that pyright raised.

I don't necessarily have any plans for any other part. I was using IntervalDict and simply decided that typehints would be useful and not to difficult to implement.

For typeshed, I am not sure. I'm not an expert on the tooling surrounding typehints, but I think distributing types within this repository itself should work fine. Based on further research, typeshed seems to be a 'final stage' for types, meant for when a decent percentage of the library is typed and the types are tested. Not adding them to typeshed is more flexible and if there are other git issues in the future, it would be easier to resolve them if stubs are locally in this repository as opposed to typeshed.

@AlexandreDecan
Copy link
Owner

Would you mind if I merge this PR in a parallel branch, so that we can first cover the other parts before "officially" releasing these changes?

@fxf8
Copy link
Author
fxf8 commented Mar 27, 2025

Okay, sure

@AlexandreDecan AlexandreDecan changed the base branch from master to typing March 27, 2025 13:23
@AlexandreDecan AlexandreDecan merged commit 695fc15 into AlexandreDecan:typing Mar 27, 2025
6 checks passed
@AlexandreDecan
Copy link
Owner

Thanks!! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0