New base class #163
New base class #163
Conversation
damskii9992
added
[scope] enhancement
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
rozyczko
left a comment
rozyczko
left a comment
There was a problem hiding this comment.
minor comments, but the whole structure looks sound
| :param skip: List of field names as strings to skip when forming the dictionary | ||
| :return: encoded object containing all information to reform an EasyScience object. | ||
| """ | ||
| serializer = SerializerBase() |
There was a problem hiding this comment.
can we instantiate SerializerBase just once in the init?
There was a problem hiding this comment.
We can.
But that would slow down class instantiation. I actually prefer to have it here, as to_dict should only be called once, if even at all. So at best we get a speed-up by only instantiating it when we need it, and at worst we get the same total time. Unless we of course serialize multiple times.
I also thought about having it in the global_object, as we only really need 1 instance of the class globally.
I also thought about making it simple functions, since most of the methods are static functions, which hints that it shouldn't even be a class.
There was a problem hiding this comment.
Keeping SerializerBase makes sense - all serialization related methods are in one location and there are enough instance methods to justify it being a separate class. If we take all the static methods away and plop them into say, serialization_utils, we would have to import them separately later and remember about that...
There was a problem hiding this comment.
It's only _convert_to_dict and its utility method _recursive_encoder which are not static methods. And both of those could just as easily have been made static methods. The class doesn't even have an __init__ or any attributes . . .
There was a problem hiding this comment.
I don't think _convert_to_dict can be easily made static...
_recursive_encoder - yes. but not the other one.
There was a problem hiding this comment.
_convert_to_dict only uses self to later refer where to pull _convert_to_dict and _recursive_encoder from. If we only use these functions from one SerializerBase encoder - they can be static. And not just static but even plain funcitons.
| cls_instance = cls(**kwargs) | ||
| for key, value in parameter_placeholder.items(): | ||
| temp_param = getattr(cls_instance, key) | ||
| setattr(cls_instance, '_' + key, value) |
There was a problem hiding this comment.
getattr and setattr could fail. Consider wrapping them in try..except
There was a problem hiding this comment.
They should only fail if we have implemented our classes incorrectly, in which case we should let them fail and throw an error to know about it, or if the serialized dict has been tampered with, in which case other pieces could fail anyways and we should also just let it fail.
Do you have a good reason as to why this could fail otherwise?
There was a problem hiding this comment.
e.g. to let users know that the failure stems from this issue. Bubbled exceptions often lose context and it's difficult to pinpoint what happened, if you're in a library code
There was a problem hiding this comment.
Is this (the new code) what you had in mind?
| This method is used by the serializer to determine which arguments are needed | ||
| by the constructor to deserialize the object. | ||
| """ | ||
| spec = getfullargspec(self.__class__.__init__) |
There was a problem hiding this comment.
It seems inspect.signature() is the modern/updated version of getfullargspec. The latter was supposed to be deprecated in >3.7 but was left for backward compatibility. We should move to modern/maintained calls.
There was a problem hiding this comment.
I replaced getfullargspec where it was somewhat simple to do an opened an issue on replacing it in the SerializerBases _convert_to_dict method here: #166
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #163 +/- ##
===========================================
+ Coverage 73.06% 80.84% +7.77%
===========================================
Files 47 50 +3
Lines 3943 4249 +306
Branches 671 739 +68
===========================================
+ Hits 2881 3435 +554
+ Misses 868 629 -239
+ Partials 194 185 -9
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| :param skip: List of field names as strings to skip when forming the dictionary | ||
| :return: encoded object containing all information to reform an EasyScience object. | ||
| """ | ||
| serializer = SerializerBase() |
There was a problem hiding this comment.
_convert_to_dict only uses self to later refer where to pull _convert_to_dict and _recursive_encoder from. If we only use these functions from one SerializerBase encoder - they can be static. And not just static but even plain funcitons.
|
|
||
| @display_name.setter | ||
| @property_stack | ||
| def display_name(self, name: str) -> None: |
There was a problem hiding this comment.
it think name hint should be optional
| new_obj = self.__class__.from_dict(temp) | ||
| return new_obj | ||
|
|
||
| def __deepcopy__(self, memo): |
There was a problem hiding this comment.
my linter complains for memo not used with the quick fix to del memo in the body of the method
There was a problem hiding this comment.
New pull request, because of weird and overlarge large changelog in #159 after rebasing on the newest develop branch.
This PR implements the changes discussed in ADR #161: A new EasyScience base class to replace BasedBase, ObjectBase, and SerializerComponent, and another base class inheriting from that to implement model-specific functionality.