Deprecate getter and setter methods of Computer properties
#4252
Conversation
sphuber
force-pushed
the
fix/2926/deprecate-computer-name
branch
from
c64dbef to
eb9b4aa
Compare
Codecov Report
@@ Coverage Diff @@
## develop #4252 +/- ##
===========================================
- Coverage 79.24% 79.23% -0.00%
===========================================
Files 468 468
Lines 34502 34559 +57
===========================================
+ Hits 27336 27379 +43
- Misses 7166 7180 +14
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
aiida/orm/computers.py
Outdated
| """ | ||
| return self._backend.computers.list_names() | ||
|
|
||
| def list_labels(self): | ||
| """Return a list with all the names of the computers in the DB.""" |
There was a problem hiding this comment.
| """Return a list with all the names of the computers in the DB.""" | |
| """Return a list with all the labels of the computers in the DB.""" |
aiida/orm/computers.py
Outdated
|
|
||
| :rtype: :class:`aiida.orm.Computer` | ||
| .. deprecated:: 1.4.0 | ||
| The keyword name will be removed in `v2.0.0`, use `label` instead. |
There was a problem hiding this comment.
| The keyword name will be removed in `v2.0.0`, use `label` instead. | |
| The `name` keyword will be removed in `v2.0.0`, use `label` instead. |
| def transport_type(self, value: str): | ||
| """Set the computer transport_type. | ||
|
|
||
| :param value: the transport_type to set. |
There was a problem hiding this comment.
| :param value: the transport_type to set. | |
| :param value: the transport type to set. |
aiida/orm/computers.py
Outdated
| Will be removed in `v2.0.0`, use the `label` property instead. | ||
| """ | ||
| warnings.warn('this property is deprecated, use `label` instead', AiidaDeprecationWarning) # pylint: disable=no-member | ||
| return self._backend_entity.name |
There was a problem hiding this comment.
you might want to use the label property instead (in case we need to change something there)
There was a problem hiding this comment.
the backend entity doesn't have a label property though. The plan was to correct this once all the name related code is ripped out just before v2.0. Since it is not public facing API we can change that at any time
There was a problem hiding this comment.
Not sure I was clear - I meant to simply replace this by return self.label, since we're repeating ourselves here which might (in principle) lead to divergence between self.label and self.get_label().
In any case, since this is just one line and is going to be removed eventually, it's just minor and I'll approve now.
| self.assertEqual(new_computer.get_mpirun_command(), options_dict_full['mpirun-command'].split()) | ||
| self.assertEqual(new_computer.get_shebang(), options_dict_full['shebang']) | ||
| self.assertEqual(new_computer.get_workdir(), options_dict_full['work-dir']) |
There was a problem hiding this comment.
What about these - shouldn't these also become properties?
There was a problem hiding this comment.
When making these changes I have followed the following:
Rule of thumb here is that if the attribute corresponds to a column on the underlying
database model, a property is used.
Since these properties here are stored in the metadata column, and in a sense are comparable to a node's attributes, I am not sure if we should give properties for all of them. This is an arbitrary choice ultimately, but one that at least tries to keep consistency with the other entities such as the Node class. The database columns are exposed through properties, while anything else gets normal methods. In the latter, we should even ask ourselves to what level we want to give individual methods to all these sub attributes. Giving too many clutters the interface, however, it does make it tab-discoverable. Putting them behind a generic Computer.get_property('shebang') makes the interface cleaner and more maintainable but maybe less discoverable. I am not sure yet what the best solution is here
sphuber
force-pushed
the
fix/2926/deprecate-computer-name
branch
from
eb9b4aa to
f433b7e
Compare
From `aiida-core==1.0.0`, we have started using properties for getters and setters of the basic attributes of ORM entities. Rule of thumb here is that if the attribute corresponds to a column on the underlying database model, a property is used. In addition, `label` is the preferred name for the third entity identifier, alongside the ID and UUID. This is already the case for most entities, except for `Computer` which is still using `name`. Here, `name` is deprecated and replaced for `label`. The changes are not yet propagated to the backend, which will be done once the deprecated resources are fully removed. This is fine because the backend is not part of the public API so doesn't have to go through a deprecation path. The `name` keyword in `Computer.objects.get` is also deprecated and replaced by `label`.
sphuber
force-pushed
the
fix/2926/deprecate-computer-name
branch
from
f433b7e to
429491d
Compare
Prepares for #2926
From
aiida-core==1.0.0, we have started using properties for gettersand setters of the basic attributes of ORM entities. Rule of thumb here
is that if the attribute corresponds to a column on the underlying
database model, a property is used.
In addition,
labelis the preferred name for the third entityidentifier, alongside the ID and UUID. This is already the case for most
entities, except for
Computerwhich is still usingname. Here,nameis deprecated and replaced forlabel. The changes are not yetpropagated to the backend, which will be done once the deprecated
resources are fully removed. This is fine because the backend is not
part of the public API so doesn't have to go through a deprecation path.
The
namekeyword inComputer.objects.getis also deprecated andreplaced by
label.