-
-
Notifications
You must be signed in to change notification settings - Fork 524
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 more type hints #1642
Add more type hints #1642
Conversation
@@ -96,7 +96,7 @@ def _encrypt(self, text: str) -> bytes: | |||
|
|||
return self.encryption_method.encrypt(text) | |||
|
|||
def open(self, filename=None): | |||
def open(self, filename: str | None = None) -> "Journal": |
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 we use Optional[str]
instead of str | None
?
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.
They are equivalent right? So we could, but I have typed this consistently as str | None
in #1614
Anything wrong with the new syntax?
@@ -17,7 +18,7 @@ def __str__(self) -> str: | |||
JRNLV2 = "Jrnlv2Encryption" | |||
|
|||
|
|||
def determine_encryption_method(config: str | bool) -> "BaseEncryption": | |||
def determine_encryption_method(config: str | bool) -> Type["BaseEncryption"]: |
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.
Nice catch here!
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 this! I think the only change is to use Optional
instead of foo | None
.
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.
lgtm
Thank you!
Add some more type hints now that #1602 is merged.
cc: #1608
Checklist
for the same issue.