Refactor models.py classes#73
Conversation
* Streamline class __init__ methods to follow more concise pattern * DSpaceObject __init__ method is also updated to prevent overwriting of links attribute if instantiated from dso param * Remove duplicative setting of links attribute from api_resource param by DSpaceObject since it is already set in the base HALResource class * Remove unnecessary __init__ methods that only call super().__init__(api_resource) * Remove unnecessary as_dict methods that do not add any fields beyond the base class fields
| self.links = api_resource.get('_links', {}).copy() if api_resource else links_default | ||
| self.embedded = api_resource.get('_embedded', {}).copy() if api_resource else {} |
There was a problem hiding this comment.
This pattern is intended to simplify the __init__ methods and hopefully ensure that a new attribute is not just partially defined
| self.lastModified = api_resource.get('lastModified') | ||
| # Python interprets _ prefix as private so for now, renaming this and handling it separately | ||
| # alternatively - each item could implement getters, or a public method to return links | ||
| self.links = api_resource.get('_links', {}).copy() |
There was a problem hiding this comment.
links was already set in the HALResource __init__ method so this was an unnecessary duplication
| # This is run after super().__init__(api_resource) to preserve any links already | ||
| # set in the dso object used for instantiation. | ||
| if dso is not None: | ||
| api_resource = dso.as_dict() | ||
| self.links = dso.links.copy() |
There was a problem hiding this comment.
This block needed to happen after the api_resource block to preventing overwriting of the dso object's links attribute
| def __init__(self, api_resource=None): | ||
| """ | ||
| Default constructor. Call DSpaceObject init then set item-specific attributes | ||
| @param api_resource: API result object to use as initial data | ||
| """ | ||
| super().__init__(api_resource) |
There was a problem hiding this comment.
Commenting here but applies to several removed blocks below, the parent __init__ method would be called if a class doesn't have it's own __init__ method so this isn't necessary. However, there may be reasons why you want to keep these __init__ methods, so let me know if that's the case.
| def as_dict(self): | ||
| """ | ||
| Return a dict representation of this Collection, based on super with collection-specific attributes added | ||
| @return: dict of Item for API use | ||
| """ | ||
| dso_dict = super().as_dict() | ||
| collection_dict = {} | ||
| return {**dso_dict, **collection_dict} |
There was a problem hiding this comment.
This also isn't necessary since it isn't adding any fields beyond the parent class so I removed a few of these as_dict methods. However, I didn't remove the Community.as_dict method since there was a #TODO about adding more fields. That may be the case with these other classes too, so I would be happy to rollback these removals if needed
| self.embedded = api_resource.get('_embedded', {}).copy() | ||
| else: | ||
| self.links = {'self': {'href': None}} | ||
| links_default = {'self': {'href': None}} |
There was a problem hiding this comment.
For this and Bitstream.checksum, I defined a var for the default dict value to improve readability
1 participant
We discovered a bug that led to the
linksattribute being overwritten with{}when aDSpaceObjectwas instantiated from thedsoparam. This prompted an examination of the class inheritance structure and how it could be streamlined to avoid this type of problem. I've successfully tested these updates with our workflows and I don't believe that any breaking changes are introduced.I'll comment on specific changes and I'm happy to undo or update any of the changes based on your feedback. Thanks!