-
-
Notifications
You must be signed in to change notification settings - Fork 498
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
typing to wb.sheets #2344
base: main
Are you sure you want to change the base?
typing to wb.sheets #2344
Conversation
It would probably be wise to take this as a trigger to split up |
Sounds good, is this PR acceptable? Next I would separate |
Hm no, I think it should be done by introducing a types module, something like here: Then the types can be imported from there. This saves us from refactoring everything and splitting up main.py just for the sake of adding types. Also, |
Ah yes you are right about the # main.py
from types import SheetType # types.py
import typing
if typing.TYPE_CHECKING:
from xlwings.main import Sheet
SheetType = typing.callable[["Sheet"],Sheet] |
Looks like There's going to be a few challenges, e.g., annotating the Collection base class so that I can do |
I have some difficulty understanding how the class App:
self.impl: base_classes.App Is valid? |
It would be different if you want to do this semi-automatically with MonkeyTyping, see #2201 |
Good idea to make it a Union instead of a super class. I understand your idea of not bothering to much with the non public API. However I wanted to understand what is happening and thought that if some of the |
581f074
to
c767c02
Compare
Hi Felix, it proves more difficult for me than previously thought. You where right that correctly typing the |
thanks! |
One remark: |
Ah does it matter for users? As type annotations are for Typecheckers. |
c767c02
to
1ef3796
Compare
Ok changed the usage to |
Hi Felix, As a rule to prevent to much OSS contrib I only have one PR per project. I know it is not feature complete but if it is suitable to merge will you merge it. As it allows me to make further contributions not typing related. |
Apologies, but it will take a while before I get to this. I need to learn more about type hints first and there's just too many other higher prios right now. Ideally, I'd like to have the whole public API fully type hinted before merging. |
I understand as using loose PR's can confuse the history a bit. On the other side if it is an improvement, you could merge it. |
related to #2201
Because I am hesitant to make a big PR with typing options because I have some questions. The
main.py
module of xlwings was ordered based on hierarchy understandably. However to reference a class as a type it has to be declarated before it is used. Therefore I movedSheet
class up.If typing would be added it could lead to reorganization of the module. Another option would be to spilt te module as an imported module would be declared first.
Using mypy as a type checker
pointed to
But matplotlib is not used within main, could it be removed or has it a function.
Non english excel
Some tests are failing due to localization differences so I don't have 100% tests passed. What would be a wise decision? Adding
skipif
if locale is not english will lead to a lot of ugly code and decreases readability.Excuse me for the overlapping non related questions 🤗