Code review comment for ~ilasc/launchpad:populate-store-upload-revision

Revision history for this message
Colin Watson (cjwatson) wrote :

There's certainly no need to rename the DB column; for this sort of thing it's always easier to handle any necessary renamings in Python, and there are various primitives available for that. The underlying DB column name isn't required to match the name of the Storm column (it's set using the 'name' parameter to the Storm variable constructor), and there are quite a few cases where we have something like this:

    _foo = Int(name='foo')

    @property
    def foo(self):
        if self._foo is not None:
            return self._foo
        else:
            # compute and return something

Of course anything that sets the actual DB column needs to use the Storm variable name, unless we also use a setter property (which can sometimes be useful), and Storm queries typically need to be written to refer to the name of the Storm variable rather than the wrapping property. The important thing is that any names used in API-exported attributes need to be defined in a way that preserves compatibility.

Happy to help with getting the details of this right tomorrow if you need it.

« Back to merge proposal