-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Stubs for boto.kms #1081
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
Stubs for boto.kms #1081
Conversation
Thanks for your contribution! I'd prefer to have types for every parameter in new stubs, but if that's too much work in this case I'll merge this PR as is. |
@JelleZijlstra Let me have a look, don't merge it for now. |
@JelleZijlstra Updated. Please check. |
Thanks! Would you mind adding some return types too? (Should have mentioned that in my first message.) Declaring the return types is helpful in having mypy typecheck your code, because otherwise any value you get back from a function in this module will just be typed as |
# | ||
# NOTE: This dynamically typed stub was automatically generated by stubgen. | ||
|
||
def regions(): ... |
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.
Returns a List[boto.regioninfo.RegionInfo]
according to the docs.
def enable_key(self, key_id: str): ... | ||
def enable_key_rotation(self, key_id: str): ... | ||
def encrypt(self, key_id: str, plaintext: bytes, encryption_context: Optional[Dict] = ..., grant_tokens: Optional[List] = ...): ... | ||
def generate_data_key(self, key_id: str, encryption_context: Optional[Dict] = ..., number_of_bytes: Optional[int] = ..., key_spec: Optional[str] = ..., grant_tokens: Optional[List] = ...): ... |
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.
The documentation says "map" as the type of the encryption_context
argument, which I assume means it accepts any mapping. Maybe we should type it as Mapping[str, Any]
? (Not sure what exactly the values are supposed to be.) In general it's also good to add type arguments for precision instead of just "Dict" or "List".
def retire_grant(self, grant_token: str): ... | ||
def revoke_grant(self, key_id: str, grant_id: str): ... | ||
def update_key_description(self, key_id: str, description: str): ... | ||
def make_request(self, key_id, description): ... |
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.
The arguments to this function are actually called "action" and "body", and if I'm reading the source correctly the types are str
and bytes
.
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.
My bad here.
@JelleZijlstra Made the changes, please check. |
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.
Just one more comment (although it applies to most return types in this file).
ResponseError = ... # type: Type[Exception] | ||
region = ... # type: Any | ||
def __init__(self, **kwargs) -> None: ... | ||
def create_alias(self, alias_name: str, target_key_id: str) -> Optional[Mapping[str, Any]]: ... |
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.
For return types, it's actually better to be precise and write Dict
if that's what it returns. As a general rule, we should be permissive in argument types and precise in return types. The first part is so that a type checker will allow people to pass in a custom dictionary-like type if they want to, and the second part is so that people who write code that mutates the dictionary returned from create_alias
don't get type errors.
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.
Makes sense. Will keep this in mind going forward. Updated.
Thanks! |
Stubs for https://github.com/boto/boto/tree/develop/boto/kms