ORM: factor out attributes/extras code into mixin classes for front- end backend #4376
Conversation
|
@sphuber thanks for all the help with the reviewing so far! I still have two questions:
|
mbercx
changed the title
mbercx
force-pushed
the
feature/attributes_extras_mixin
branch
2 times, most recently
from
0832962 to
bcfad38
Compare
Codecov Report
@@ Coverage Diff @@
## develop #4376 +/- ##
===========================================
- Coverage 79.32% 79.30% -0.02%
===========================================
Files 468 470 +2
Lines 34757 34635 -122
===========================================
- Hits 27569 27465 -104
+ Misses 7188 7170 -18
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
mbercx
force-pushed
the
feature/attributes_extras_mixin
branch
from
bcfad38 to
2326620
Compare
|
I could have sworn I typed what I am about to type yesterday evening already, but apparently it didn't get through. So I will redo it :)
Right back at you! Great work so far.
I see your point, but there is an additional advantage of separating it into a separate file: it actually makes the code more readable. As it stands, the
I have no idea. It looks fine. This might actually just be a pylint bug. These are notoriously hard to fix or simply even get rid off. I will try to do some testing locally. In the meantime, since this PR will need to keep more than 1 commit, and so I won't be able to squash, it will be important to have the commits exactly as they will have to be merged. Your first two commits are already perfect. I would just suggest to add a third on top of that, that simply creates the front end mixins. Then you just have to put the fixes of the additional commits and rebase them in the respective commits where they are supposed to go. Finally, I would maybe prefix each commit title with "ORM:" such that it is immediately clear what part of the code it touches.
|
mbercx
force-pushed
the
feature/attributes_extras_mixin
branch
3 times, most recently
from
edd041c to
a6d5354
Compare
Agreed, the
My local pre-commit runs don't actually raise this issue. Might be related to the pylint version we are using?
Done! Let me know if you have any further comments. |
aiida/orm/implementation/entities.py
Outdated
|
|
||
| from aiida.orm.implementation.utils import clean_value, validate_attribute_extra_key | ||
|
|
||
| __all__ = ('BackendEntity', 'BackendCollection', 'EntityType', 'BackendEntityAttributesMixin', \ |
There was a problem hiding this comment.
Just small reference for the future (don't need to adapt it here): the slash is not necessary when you are in between parentheses.
|
Thanks a lot @mbercx , this is perfect. All we need to do is figure out this pylint bug and then rebase it on latest develop and we can merge this. I will also see if I can find a solution for the pylint thing, but will have to wait until Tuesday. |
Make sure the code for the attributes and extras methods are identical, as a first step towards refactoring the code to use a mixin class for these methods. These changes should have no influence on how the methods function. Add exception chaining for exceptions raised directly during the handling of another exception. There is only a minor difference in the output, but it should make it clear that this exception was raised purposefully.
sphuber
force-pushed
the
feature/attributes_extras_mixin
branch
4 times, most recently
from
69a9a66 to
0b556a7
Compare
Move the attributes and extras methods to two mixin classes called `BackendEntityAttributesMixin` and `BackendEntityExtrasMixin`, stored in the new aiida.orm.implementation.entities.py module. The mixin classes rely on the `is_stored` and `_flush_if_stored` methods, so these are added as abstract methods. They are "mixed in" at the BackendNode` level where the abstract methods of the attributes and extras are removed. Move the `_flush_if_stored` method to the `BackendEntity` class, which is added leftmost to the `BackendNode` parent classes. This method can be used by all backend entities. Move `BackendEntity` and `BackendCollection` classes to `aiida.orm.implementation.entities.py` module. Move `validate_attribute_extra_key` and `clean_value` methods to new module `aiida.orm.implementation.utils.py`. Move the calls to `validates_attribute_extra_key` method from the front end `Node` class to the backend mixin classes `AttributesBackendEntity` and `ExtrasBackendEntity`. This way the key/value validation/cleaning is both done at the backend level, which is more consistent. Moreover, this means other frontend classes won't have to add this call to `validates_attribute_extra_key` to their methods, when they want to use the attributes/extras methods. Add exception chaining for all the modules that are adjusted.
Move all methods related to attributes and extras from the frontend `Node` class to separate mixin classes called `EntityAttributesMixin` and `EntityExtrasMixin`. This makes it easier to add these methods to other frontend entity classes and makes the code more maintainable.
sphuber
force-pushed
the
feature/attributes_extras_mixin
branch
from
0b556a7 to
f2dc964
Compare
sphuber
changed the title
There was a problem hiding this comment.
Thanks a lot @mbercx ! The final problem with the pylint false positive on the __all__ in the aiida.orm.implementation.backends had to do with the fact that the aiida.orm.implementation.sql module also defines a backends module and imports the base backends. Some complex process of import order and potential overwriting, caused pylint to not understand what was going on. Removing the __all__ from the aiida.orm.implementation.sql module fixed it. It is ok because that module is not used outside the backend and doesn't really need a shorthand import path
Currently, the methods for
attributesandextrasof theNodebackend entity classes are implemented for both the Django and SqlAlchemy backend classes (DjangoNodeandSqlaNode). Since the methods are very similar, it's better to homogenize them and introduce mixin classes which we can add as parents to theBackendNodeclass. This has the added advantage that in case we want to addattributesorextrasto other entities, we only have to add either mixin class as a parent to the backend entity class to achieve this.We can do the same for the front-end
Nodeclass, however here we only create the mixin class for theextras, as we are not planning to addattributesto other entities for now, we only add theEntityExtrasMixinclass, since the plan is to addextrasto theGroupentity (see #4328).This PR does the following:
DjangoNodeandSqlaNodebackend classes.DjangoNodeandSqlaNodebackend classes related toattributesandextrasto two mixin classes: AttributesBackendEntity and ExtrasBackendEntity. Add these as parent classes to theBackendNodeclass.`Nodefront-end class related toextrasto theEntityExtrasMixinclass.