Data: allow source to be passed as a keyword argument
#5163
Conversation
sphuber
force-pushed
the
fix/5162/data-constructor-source-attribute
branch
from
f8d324a
to
5718650
Compare
Codecov Report
@@ Coverage Diff @@
## develop #5163 +/- ##
===========================================
- Coverage 80.93% 80.93% -0.00%
===========================================
Files 536 536
Lines 37056 37054 -2
===========================================
- Hits 29989 29985 -4
- Misses 7067 7069 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| def __init__( | ||
| self, | ||
| ase=None, | ||
| file=None, | ||
| filename=None, | ||
| values=None, | ||
| source=None, | ||
| scan_type=None, | ||
| parse_policy=None, | ||
| **kwargs | ||
| ): | ||
| def __init__(self, ase=None, file=None, filename=None, values=None, scan_type=None, parse_policy=None, **kwargs): | ||
| """Construct a new instance and set the contents to that of the file. | ||
|
|
||
| :param file: an absolute filepath or filelike object for CIF. | ||
| Hint: Pass io.BytesIO(b"my string") to construct the SinglefileData directly from a string. | ||
| :param filename: specify filename to use (defaults to name of provided file). | ||
| :param ase: ASE Atoms object to construct the CifData instance from. | ||
| :param values: PyCifRW CifFile object to construct the CifData instance from. | ||
| :param source: |
There was a problem hiding this comment.
If I understand correctly the source would be identified here as part of the kwargs and passed down to super().__init__, but since we are "overwriting" the __init__ wouldn't we loose the documentation aspect if we just remove this (this=the mention in the docstring and the explicit arg declaration)?
There was a problem hiding this comment.
Yes, we would lose it, but it shouldn't have been there in the first place. The fact that you can pass source is also not in the docstring of any of all the other data plugins. This is something that should simply be documented in the documentation. Docstrings are not the place for this. This is a generic problem with any kind of subclassing system that allows to change signatures.
So I guess that is the real thing missing from this PR. I will look if I can add a section in the docs to document this feature.
There was a problem hiding this comment.
Docstrings are not the place for this.
Yeah, I'm still not super sure what docstrings are the place for really. With all the format limitations, the lack of support for expanding docstrings of parent classes instead of just overwriting, etc. I feel like they are quite limited and I am more and more hesitant to dedicate a lot of effort into them.
The `Data` class defines the `source` property which allows to get and set the `source` attribute. It was designed to hold information on the source of the data if it came directly from an external database. Prior to `aiida-core` v1.0 it was possible to set this attribute directly through the constructor as a keyword argument, but due to the refactoring of the ORM, this was no longer possible. It went undetected because it was never tested and the two data types where this feature is typically used, the `UpfData` and `CifData`, implemented the keyword argument themselves, hiding the fact that `Data` no longer supported it. The `Data` constructor signature is updated to once again recognize the `source` keyword argument, and the custom implementation is removed from the `UpfData` and `CifData` plugins which now simply rely on the super class. A section is added to the "How to work with Data" section in the docs to document this feature and to recommend its usage when constructing data nodes by hand from external databases.
sphuber
force-pushed
the
fix/5162/data-constructor-source-attribute
branch
from
5718650
to
fac5558
Compare
|
@ramirezfranciscof thanks for the review. I have added a section to the documentation explaining this feature |
Fixes #5162
The
Dataclass defines thesourceproperty which allows to get andset the
sourceattribute. It was designed to hold information on thesource of the data if it came directly from an external database.
Prior to
aiida-corev1.0 it was possible to set this attributedirectly through the constructor as a keyword argument, but due to the
refactoring of the ORM, this was no longer possible. It went undetected
because it was never tested and the two data types where this feature is
typically used, the
UpfDataandCifData, implemented the keywordargument themselves, hiding the fact that
Datano longer supported it.The
Dataconstructor signature is update to once again recognize thesourcekeyword argument, and the custom implementation is removed fromthe
UpfDataandCifDataplugins which now simply rely on the superclass.