-
Notifications
You must be signed in to change notification settings - Fork 14
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
Miscellaneous python typing fixes #6122
Changes from all commits
5b7654a
6c510e2
cb6920f
150f511
214233a
c9ea77c
c602a1c
44b6402
26feb59
8d6c4c4
60a33e5
d18fd65
2d0e9ff
b0daa88
2230f35
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,7 +100,7 @@ def set(self, group, key, value): | |
# pylint:disable=unsupported-assignment-operation | ||
super().setdefault(group, {})[key] = value | ||
|
||
def set_secret(self, aes_service, group, key, value: str): | ||
def set_secret(self, aes_service, group, key, value: str) -> None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's unfortunate that there is no type inference for return types, even functions that return |
||
""" | ||
Store a setting as a secret. | ||
|
||
|
@@ -112,12 +112,14 @@ def set_secret(self, aes_service, group, key, value: str): | |
:param value: The value to set | ||
""" | ||
aes_iv = aes_service.build_iv() | ||
value = aes_service.encrypt(aes_iv, value) | ||
encrypted_value: bytes = aes_service.encrypt(aes_iv, value) | ||
|
||
# Store both the setting and the IV | ||
# We can't store the bytes directly in JSON so we store it as base64 | ||
# pylint:disable=unsupported-assignment-operation | ||
super().setdefault(group, {})[key] = base64.b64encode(value).decode("utf-8") | ||
super().setdefault(group, {})[key] = base64.b64encode(encrypted_value).decode( | ||
"utf-8" | ||
) | ||
super().setdefault(group, {})[f"{key}_aes_iv"] = base64.b64encode( | ||
aes_iv | ||
).decode("utf-8") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,7 +28,7 @@ class PublicId: | |
app_code: str = "lms" | ||
"""Code representing the product this model is in.""" | ||
|
||
instance_id: str = None | ||
instance_id: str | None = None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as above about data class fields which are only null during initialization. It would be good to find a pattern so code which only handles initialized instances doesn't need unnecessary non-null checks. |
||
"""Identifier for the specific model instance.""" | ||
|
||
def __post_init__(self): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,14 +3,14 @@ | |
from lms.product.blackboard._plugin.course_copy import BlackboardCourseCopyPlugin | ||
from lms.product.blackboard._plugin.grouping import BlackboardGroupingPlugin | ||
from lms.product.blackboard._plugin.misc import BlackboardMiscPlugin | ||
from lms.product.product import PluginConfig, Product, Routes | ||
from lms.product.product import Family, PluginConfig, Product, Routes | ||
|
||
|
||
@dataclass | ||
class Blackboard(Product): | ||
"""A product for Blackboard specific settings and tweaks.""" | ||
|
||
family: Product.Family = Product.Family.BLACKBOARD | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Access these more directly |
||
family: Family = Family.BLACKBOARD | ||
|
||
route: Routes = Routes( | ||
oauth2_authorize="blackboard_api.oauth.authorize", | ||
|
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.
Using
str | None
here has the downside that mypy will ask you to add checks that this is non-null whenever using an initialized instance. If I understand correctly, this value is onlyNone
during construction? There are some Stack Overflow posts discussing this issue, such as https://stackoverflow.com/q/74621969/434243.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.
I'll keep looking for a good solution in a different PR.
The feature we are after is for dataclass fields that can be used during init but post__init provides a default if that's missing.