Skip to content

refactor type hinting #44

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.

Sign up for GitHub

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

Closed
jaymccon wants to merge 1 commit into from
Closed

refactor type hinting #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion python/rpdk/python/codegen.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from rpdk.core.jsonutils.resolver import ContainerType, resolve_models
from rpdk.core.plugin_base import LanguagePlugin

from .resolver import models_in_properties, translate_type
from .resolver import models_in_properties, resource_name_suffix, translate_type

LOG = logging.getLogger(__name__)

Expand Down Expand Up @@ -42,6 +42,7 @@ def __init__(self):
trim_blocks=True, lstrip_blocks=True, keep_trailing_newline=True
)
self.env.filters["translate_type"] = translate_type
self.env.filters["resource_name_suffix"] = resource_name_suffix
self.env.filters["models_in_properties"] = models_in_properties
self.env.globals["ContainerType"] = ContainerType
self.namespace = None
Expand Down
11 changes: 9 additions & 2 deletions python/rpdk/python/resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,17 @@
}


def resource_name_suffix(name):
# add a suffix to prevent typing conflicts
if name != "ResourceModel":
return f"{name}ResourceModel"

@tobywf tobywf Nov 8, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's only one resource model, so this is confusing. it's also pretty heavyweight. maybe something like we do in the Java plugin in a simpler solution? (models already are guaranteed to have unique names, so you only have to disambiguate clashes with languages types. as far as i can see, the same is true for type hints)

@tobywf tobywf Nov 19, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I see why you may have done this. interesting mypy errors happen when a variable has the same name as a type (the variable shadows the type for type-hints only)

return name


def translate_type(resolved_type):
if resolved_type.container == ContainerType.MODEL:
# we create type vars to make it easier to ref later models
return f"T{resolved_type.type}"
# quote types to ensure they can be ref'd
return f'"{resource_name_suffix(resolved_type.type)}"'
if resolved_type.container == ContainerType.PRIMITIVE:
return PRIMITIVE_TYPES[resolved_type.type]

Expand Down
29 changes: 14 additions & 15 deletions python/rpdk/python/templates/handlers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@
OperationStatus,
ProgressEvent,
Resource,
ResourceHandlerRequest,
SessionProxy,
exceptions,
)

from .models import ResourceModel, TResourceModel
from .models import ResourceHandlerRequest, ResourceModel

# Use this logger to forward log messages to CloudWatch Logs.
LOG = logging.getLogger(__name__)
Expand All @@ -24,11 +23,11 @@
@resource.handler(Action.CREATE)
def create_handler(
session: Optional[SessionProxy],
request: ResourceHandlerRequest[TResourceModel],
request: ResourceHandlerRequest,
callback_context: MutableMapping[str, Any],
) -> ProgressEvent[TResourceModel]:
) -> ProgressEvent:
model = request.desiredResourceState
progress: ProgressEvent[TResourceModel] = ProgressEvent(
progress: ProgressEvent = ProgressEvent(
status=OperationStatus.IN_PROGRESS,
resourceModel=model,
)
Expand All @@ -51,11 +50,11 @@ def create_handler(
@resource.handler(Action.UPDATE)
def update_handler(
session: Optional[SessionProxy],
request: ResourceHandlerRequest[TResourceModel],
request: ResourceHandlerRequest,
callback_context: MutableMapping[str, Any],
) -> ProgressEvent[TResourceModel]:
) -> ProgressEvent:
model = request.desiredResourceState
progress: ProgressEvent[TResourceModel] = ProgressEvent(
progress: ProgressEvent = ProgressEvent(
status=OperationStatus.IN_PROGRESS,
resourceModel=model,
)
Expand All @@ -66,11 +65,11 @@ def update_handler(
@resource.handler(Action.DELETE)
def delete_handler(
session: Optional[SessionProxy],
request: ResourceHandlerRequest[TResourceModel],
request: ResourceHandlerRequest,
callback_context: MutableMapping[str, Any],
) -> ProgressEvent[TResourceModel]:
) -> ProgressEvent:
model = request.desiredResourceState
progress: ProgressEvent[TResourceModel] = ProgressEvent(
progress: ProgressEvent = ProgressEvent(
status=OperationStatus.IN_PROGRESS,
resourceModel=model,
)
Expand All @@ -81,9 +80,9 @@ def delete_handler(
@resource.handler(Action.READ)
def read_handler(
session: Optional[SessionProxy],
request: ResourceHandlerRequest[TResourceModel],
request: ResourceHandlerRequest,
callback_context: MutableMapping[str, Any],
) -> ProgressEvent[TResourceModel]:
) -> ProgressEvent:
model = request.desiredResourceState
# TODO: put code here
return ProgressEvent(
Expand All @@ -95,9 +94,9 @@ def read_handler(
@resource.handler(Action.LIST)
def list_handler(
session: Optional[SessionProxy],
request: ResourceHandlerRequest[TResourceModel],
request: ResourceHandlerRequest,
callback_context: MutableMapping[str, Any],
) -> ProgressEvent[TResourceModel]:
) -> ProgressEvent:
# TODO: put code here
return ProgressEvent(
status=OperationStatus.SUCCESS,
Expand Down
38 changes: 21 additions & 17 deletions python/rpdk/python/templates/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{%- set used_models = properties|models_in_properties -%}
{%- if used_models -%}(
{%- for name in used_models -%}
Generic[T{{ name }}]{%- if not loop.last -%}, {%- endif -%}
"{{ name }}"{%- if not loop.last -%}, {%- endif -%}

@tobywf tobywf Nov 8, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since your code no longer uses the class_model_bindings or typevar_model_bindings macros, can't you just delete them?

{%- endfor -%}
){%- endif -%}
{%- endmacro -%}
Expand All @@ -24,45 +24,49 @@
TypeVar,
)

T = TypeVar("T")
from aws_cloudformation_rpdk_python_lib.interface import (
BaseResourceHandlerRequest,
BaseResourceModel,
)


def set_or_none(value: Optional[Sequence[T]]) -> Optional[AbstractSet[T]]:
def set_or_none(value: Optional[Sequence[Any]]) -> Optional[AbstractSet[Any]]:

@tobywf tobywf Nov 8, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you keep T = TypeVar("T"), you can leave this as T and provide stronger type guarantees

if value:
return set(value)
return None


{% for model, properties in models.items() %}
T{{ model }} = TypeVar("T{{ model }}", bound="{{ model }}{{ typevar_model_bindings(properties) }}")
{% endfor %}
@dataclass
class ResourceHandlerRequest(BaseResourceHandlerRequest):
# pylint: disable=invalid-name
desiredResourceState: Optional["ResourceModel"]
previousResourceState: Optional["ResourceModel"]


{% for model, properties in models.items() %}


@dataclass
class {{ model }}{{ class_model_bindings(properties) }}:
class {{ model|resource_name_suffix }}(BaseResourceModel):

@tobywf tobywf Nov 8, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sub-models aren't and shouldn't be valid instances of BaseResourceModel, so only the actual resource model should inherit from BaseResourceModel, right?

{% for name, type in properties.items() %}
{{ name }}: Optional[{{ type|translate_type }}]
{% endfor %}

def _serialize(self) -> Mapping[str, Any]:
return self.__dict__

@classmethod
def _deserialize(
cls: Type[T{{ model }}],
json: Mapping[str, Any],
) -> Optional[T{{ model }}]:
if not json:
cls: Type["{{ model|resource_name_suffix }}"],
json_data: Optional[Mapping[str, Any]],
) -> Optional["{{ model|resource_name_suffix }}"]:
if not json_data:
return None
return cls(
{% for name, type in properties.items() %}
{% if type.container == ContainerType.MODEL %}
{{ name }}={{ type.type }}._deserialize(json.get("{{ name }}")), # type: ignore
{{ name }}={{ type.type|resource_name_suffix }}._deserialize(json_data.get("{{ name }}")),
{% elif type.container == ContainerType.SET %}
{{ name }}=set_or_none(json.get("{{ name }}")),
{{ name }}=set_or_none(json_data.get("{{ name }}")),
{% else %}
{{ name }}=json.get("{{ name }}"),
{{ name }}=json_data.get("{{ name }}"),
{% endif %}
{% endfor %}
)
Expand Down
2 changes: 1 addition & 1 deletion src/aws_cloudformation_rpdk_python_lib/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@
from .boto3_proxy import SessionProxy # noqa: F401
from .interface import ( # noqa: F401
Action,
BaseResourceHandlerRequest,
HandlerErrorCode,
OperationStatus,
ProgressEvent,
ResourceHandlerRequest,
)
from .resource import Resource # noqa: F401

Expand Down
36 changes: 18 additions & 18 deletions src/aws_cloudformation_rpdk_python_lib/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,76 +1,76 @@
from typing import Any, Generic
from typing import Any

from .interface import HandlerErrorCode, ProgressEvent, T
from .interface import HandlerErrorCode, ProgressEvent


class _HandlerError(Exception, Generic[T]):
class _HandlerError(Exception):
def __init__(self, *args: Any):
self._error_code = HandlerErrorCode[type(self).__name__]
super().__init__(*args)

def to_progress_event(self) -> ProgressEvent[T]:
def to_progress_event(self) -> ProgressEvent:
return ProgressEvent.failed(self._error_code, str(self))


class NotUpdatable(_HandlerError[T]):
class NotUpdatable(_HandlerError):
pass


class InvalidRequest(_HandlerError[T]):
class InvalidRequest(_HandlerError):
pass


class AccessDenied(_HandlerError[T]):
class AccessDenied(_HandlerError):
pass


class InvalidCredentials(_HandlerError[T]):
class InvalidCredentials(_HandlerError):
pass


class AlreadyExists(_HandlerError[T]):
class AlreadyExists(_HandlerError):
def __init__(self, type_name: str, identifier: str):
super().__init__(
f"Resource of type '{type_name}' with identifier "
f"'{identifier}' already exists."
)


class NotFound(_HandlerError[T]):
class NotFound(_HandlerError):
def __init__(self, type_name: str, identifier: str):
super().__init__(
f"Resource of type '{type_name}' with identifier "
f"'{identifier}' was not found."
)


class ResourceConflict(_HandlerError[T]):
class ResourceConflict(_HandlerError):
pass


class Throttling(_HandlerError[T]):
class Throttling(_HandlerError):
pass


class ServiceLimitExceeded(_HandlerError[T]):
class ServiceLimitExceeded(_HandlerError):
pass


class NotStabilized(_HandlerError[T]):
class NotStabilized(_HandlerError):
pass


class GeneralServiceException(_HandlerError[T]):
class GeneralServiceException(_HandlerError):
pass


class ServiceInternalError(_HandlerError[T]):
class ServiceInternalError(_HandlerError):
pass


class NetworkFailure(_HandlerError[T]):
class NetworkFailure(_HandlerError):
pass


class InternalFailure(_HandlerError[T]):
class InternalFailure(_HandlerError):
pass
31 changes: 20 additions & 11 deletions src/aws_cloudformation_rpdk_python_lib/interface.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
import logging
from dataclasses import dataclass
from enum import Enum, auto
from typing import Any, Generic, List, Mapping, MutableMapping, Optional, Type, TypeVar
from typing import Any, List, Mapping, MutableMapping, Optional, Type

LOG = logging.getLogger(__name__)

T = TypeVar("T") # pylint: disable=invalid-name


class _AutoName(Enum):
@staticmethod
Expand Down Expand Up @@ -48,17 +46,28 @@ class HandlerErrorCode(str, _AutoName):
InternalFailure = auto()


class BaseResourceModel:
def _serialize(self) -> Mapping[str, Any]:
return self.__dict__

@classmethod
def _deserialize(
cls: Type["BaseResourceModel"], json_data: Optional[Mapping[str, Any]]
) -> Optional["BaseResourceModel"]:
raise NotImplementedError()


# pylint: disable=too-many-instance-attributes
@dataclass
class ProgressEvent(Generic[T]):
class ProgressEvent:
# pylint: disable=invalid-name
status: OperationStatus
errorCode: Optional[HandlerErrorCode] = None
message: str = ""
callbackContext: Optional[Mapping[str, Any]] = None
callbackDelaySeconds: int = 0
resourceModel: Optional[T] = None
resourceModels: Optional[List[T]] = None
resourceModel: Optional[BaseResourceModel] = None
resourceModels: Optional[List[BaseResourceModel]] = None
nextToken: Optional[str] = None

def _serialize(
Expand All @@ -77,16 +86,16 @@ def _serialize(

@classmethod
def failed(
cls: Type["ProgressEvent[T]"], error_code: HandlerErrorCode, message: str
) -> "ProgressEvent[T]":
cls: Type["ProgressEvent"], error_code: HandlerErrorCode, message: str
) -> "ProgressEvent":
return cls(status=OperationStatus.FAILED, errorCode=error_code, message=message)


@dataclass
class ResourceHandlerRequest(Generic[T]):
class BaseResourceHandlerRequest:
# pylint: disable=invalid-name
clientRequestToken: str
desiredResourceState: Optional[T]
previousResourceState: Optional[T]
desiredResourceState: Optional[BaseResourceModel]
previousResourceState: Optional[BaseResourceModel]
logicalResourceIdentifier: Optional[str]
nextToken: Optional[str]
Loading