adding some type hints and few type checks #55
Conversation
src/poppler/document.py
Outdated
| ) | ||
|
|
||
|
|
||
| def load_from_data(file_data: bytes, owner_password=None, user_password=None): | ||
| def load_from_data( | ||
| file_data: bytes, owner_password: str = "", user_password: str = "" |
There was a problem hiding this comment.
I prefer using None for default It is more explicit. With you suggestion, it is no longer possible to use None as argument to the function.
There was a problem hiding this comment.
Apart from that, usually using None means no password, while "" could be an empty one. Don't know, how poppler/pdf deals with empty passwords...
There was a problem hiding this comment.
I don't know much about PDFs but if empty password is a valid password for a PDF then I agree with you being None for default. otherwise it is better to use empty string as default.
Waiting for your comment @cbrunet
There was a problem hiding this comment.
The C++ API expects an empty string. However, semantically, in Python, None means no value. Since we are in the context of a library, I prefer to let the user choose the value that is more suitable for him. Furthermore, annotating the password field as Optional[str] is more expressive, imho.
|
So you are asking me to add type hints to all files? |
src/poppler/document.py
Outdated
| ) | ||
|
|
||
|
|
||
| def load_from_data(file_data: bytes, owner_password=None, user_password=None): | ||
| def load_from_data( | ||
| file_data: bytes, owner_password: str = "", user_password: str = "" |
There was a problem hiding this comment.
The C++ API expects an empty string. However, semantically, in Python, None means no value. Since we are in the context of a library, I prefer to let the user choose the value that is more suitable for him. Furthermore, annotating the password field as Optional[str] is more expressive, imho.
yes, if it is something you'd like to do, it would be a great contribution. I would not add any explicit type checks however. |
Currently, I don't have time to do the type hints for all of the API. And I suppose having type hints for some parts is better than not having them at all. |
|
@cbrunet any other change required? EDIT: Sorry I forgot it for a long time... |
|
@cbrunet enough sleep! wake up! |
Hello there.
I have added some Python type hints and a few type checks.
Please let me know if I should change anything.
Regards, Farooq.