aiidateam / aiida-core

The official repository for the AiiDA code
https://aiida-core.readthedocs.io
Other
431 stars 187 forks source link

`NodeRepositoryMixin._update_repository_metadata` makes no sense 😝 #4936

Open chrisjsewell opened 3 years ago

chrisjsewell commented 3 years ago

Apologies if I'm missing something here, but this is the method:

    def _update_repository_metadata(self):
        """Refresh the repository metadata of the node if it is stored and the decorated method returns successfully."""
        if self.is_stored:
            self.repository_metadata = self._repository.serialize()

so it only mutates a stored node: which does not seem a good idea to start with?

but then... in every place that its called, there's first a mutability check that self.is_stored is False, e.g.

    def erase(self):
        """Delete all objects from the repository.

        :raises `~aiida.common.exceptions.ModificationNotAllowed`: when the node is stored and therefore immutable.
        """
        self.check_mutability()
        self._repository.erase()
        self._update_repository_metadata()

so it surely it has no purpose??

there is also the NodeRepositoryMixin docstring seems to confirm that repository_metadata is not intended to be updated until just before it is stored:

This layer explicitly does not update the metadata of the node on a mutation action. The reason is that for stored nodes these actions are anyway forbidden and for unstored nodes, the final metadata will be stored in one go, once the node is stored, so there is no need to keep updating the node metadata intermediately. Note that this does mean that repository_metadata does not give accurate information as long as the node is not yet stored.

chrisjsewell commented 3 years ago

@sphuber please tell me I'm not going mad lol