refactor: introduce AbstractResource base class for resource interface - #175
refactor: introduce AbstractResource base class for resource interface#175phantomii wants to merge 1 commit into
Conversation
Summary of ChangesHello @phantomii, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the resource management architecture by introducing a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an AbstractResource base class to establish a common interface for resource objects, which is a positive step for the codebase's architecture. The refactoring of Resource and ImportedResource to use this new abstract class, and the removal of __getattr__ in favor of explicit delegation, are good changes for clarity and type safety. My review includes suggestions to further improve type safety by adding missing type hints, which aligns with the goals of this PR. I have also identified a critical bug in the implementation of ImportedResource.get_parameter_value that results from the refactoring, and I've provided a suggested fix.
| def get_parameter_value(self, parameter): | ||
| return type(self._resource).get_parameter_value(self, parameter) |
There was a problem hiding this comment.
This implementation is broken after removing __getattr__. The call type(self._resource).get_parameter_value(self, parameter) will execute Resource.get_parameter_value on an ImportedResource instance (self). This will raise an AttributeError because ImportedResource does not have attributes like actual_resource or __allowed_methods_from_manifest__ which are accessed within that method.
A simple delegation to self._resource.get_parameter_value(parameter) is also incorrect because the resource name check inside it would use the original resource's name instead of the imported resource's name.
I suggest reconstructing the parameter string to use the original resource's name and then delegating the call. This correctly reuses the logic without code duplication and fixes the bug.
| def get_parameter_value(self, parameter): | |
| return type(self._resource).get_parameter_value(self, parameter) | |
| def get_parameter_value(self, parameter): | |
| parts = parameter.split(":") | |
| resource_name = parts[0][1:] | |
| if resource_name != self.name: | |
| raise ValueError( | |
| f"Resource name `{resource_name}` does not match the" | |
| f" current resource name `{self.name}`" | |
| ) | |
| new_parameter = f"${self._resource.name}:{':'.join(parts[1:])}" | |
| return self._resource.get_parameter_value(new_parameter) |
| class AbstractResource(metaclass=abc.ABCMeta): | ||
|
|
||
| @abc.abstractmethod | ||
| def get_uri(self): | ||
| raise NotImplementedError("Not implemented") | ||
|
|
||
| @abc.abstractmethod | ||
| def to_str(self, field: str) -> str: | ||
| raise NotImplementedError("Not implemented") | ||
|
|
||
| @abc.abstractmethod | ||
| def get_parameter_value(self, parameter): | ||
| raise NotImplementedError("Not implemented") | ||
|
|
||
| @abc.abstractmethod | ||
| def get_actual_state_safe(self): | ||
| raise NotImplementedError("Not implemented") | ||
|
|
||
| @abc.abstractmethod | ||
| def render_target_state(self, engine=None): | ||
| raise NotImplementedError("Not implemented") | ||
|
|
||
| @property | ||
| @abc.abstractmethod | ||
| def link(self): | ||
| raise NotImplementedError("Not implemented") | ||
|
|
||
| @abc.abstractmethod | ||
| def get_provider_element(self): | ||
| raise NotImplementedError("Not implemented") | ||
|
|
||
| @property | ||
| @abc.abstractmethod | ||
| def kind(self): | ||
| raise NotImplementedError("Not implemented") | ||
|
|
||
| @abc.abstractmethod | ||
| def calculate_full_hash(self): | ||
| raise NotImplementedError("Not implemented") | ||
|
|
||
| @abc.abstractmethod | ||
| def actualize(self): | ||
| raise NotImplementedError("Not implemented") | ||
|
|
||
| @abc.abstractmethod | ||
| def delete(self, session=None): | ||
| raise NotImplementedError("Not implemented") | ||
|
|
||
| @property | ||
| @abc.abstractmethod | ||
| def original(self): | ||
| raise NotImplementedError("Not implemented") |
There was a problem hiding this comment.
To improve type safety and align with the PR's goal, it would be beneficial to add type hints to the abstract methods. This will make the interface clearer for implementers. You may need to import typing as tp and use forward references (string quotes) for types defined later in the file or for circular dependencies.
class AbstractResource(metaclass=abc.ABCMeta):
@abc.abstractmethod
def get_uri(self) -> str:
raise NotImplementedError("Not implemented")
@abc.abstractmethod
def to_str(self, field: str) -> str:
raise NotImplementedError("Not implemented")
@abc.abstractmethod
def get_parameter_value(self, parameter: str) -> 'tp.Any':
raise NotImplementedError("Not implemented")
@abc.abstractmethod
def get_actual_state_safe(self) -> dict:
raise NotImplementedError("Not implemented")
@abc.abstractmethod
def render_target_state(self, engine: 'ElementEngine' | None = None) -> dict:
raise NotImplementedError("Not implemented")
@property
@abc.abstractmethod
def link(self) -> str:
raise NotImplementedError("Not implemented")
@abc.abstractmethod
def get_provider_element(self) -> 'Element':
raise NotImplementedError("Not implemented")
@property
@abc.abstractmethod
def kind(self) -> str:
raise NotImplementedError("Not implemented")
@abc.abstractmethod
def calculate_full_hash(self) -> str:
raise NotImplementedError("Not implemented")
@abc.abstractmethod
def actualize(self) -> None:
raise NotImplementedError("Not implemented")
@abc.abstractmethod
def delete(self, session: 'tp.Any' | None = None) -> None:
raise NotImplementedError("Not implemented")
@property
@abc.abstractmethod
def original(self) -> 'AbstractResource':
raise NotImplementedError("Not implemented")| def get_uri(self): | ||
| return self._resource.get_uri() | ||
|
|
||
| def to_str(self, field: str) -> str: | ||
| return self._resource.to_str(field) | ||
|
|
||
| def get_actual_state_safe(self): | ||
| return self._resource.get_actual_state_safe() | ||
|
|
||
| def render_target_state(self, engine=None): | ||
| return self._resource.render_target_state(engine) | ||
|
|
||
| def get_provider_element(self): | ||
| return self._resource.get_provider_element() | ||
|
|
||
| @property | ||
| def kind(self): | ||
| return self._resource.kind | ||
|
|
||
| def calculate_full_hash(self): | ||
| return self._resource.calculate_full_hash() | ||
|
|
||
| def actualize(self): | ||
| return self._resource.actualize() | ||
|
|
||
| def delete(self, session=None): | ||
| return self._resource.delete(session) | ||
|
|
||
| @property | ||
| def original(self): | ||
| return self._resource |
There was a problem hiding this comment.
For consistency and improved type safety, consider adding type hints to these delegated methods, similar to how to_str is typed. This will make the ImportedResource class more robust and easier to use correctly. You may need to import typing as tp.
def get_uri(self) -> str:
return self._resource.get_uri()
def to_str(self, field: str) -> str:
return self._resource.to_str(field)
def get_actual_state_safe(self) -> dict:
return self._resource.get_actual_state_safe()
def render_target_state(self, engine: 'ElementEngine' | None = None) -> dict:
return self._resource.render_target_state(engine)
def get_provider_element(self) -> 'Element':
return self._resource.get_provider_element()
@property
def kind(self) -> str:
return self._resource.kind
def calculate_full_hash(self) -> str:
return self._resource.calculate_full_hash()
def actualize(self) -> None:
return self._resource.actualize()
def delete(self, session: 'tp.Any' | None = None) -> None:
return self._resource.delete(session)
@property
def original(self) -> 'Resource':
return self._resource
- Create AbstractResource class defining common resource interface - Update Resource class to inherit from AbstractResource - Refactor ImportedResource to implement AbstractResource interface - Remove __getattr__ delegation in favor of explicit method implementations - Improve code architecture and type safety for resource management
5dc1238 to
97bffa2
Compare
1 participant
__getattr__delegation in favor of explicit method implementations