Code review comment for lp:~bryce/launchpad/lp-617695-linkui

Revision history for this message
Bryce Harrington (bryce) wrote :

@Tim, I've promoted the Store code from the browser to a proper interface routine, and generalized it a bit so it'll be useful in and of itself. The remaining code in updateContextFromData() seems harder to abstract away, since it'd make the error messages less meaningful. Note that the code that actually does the linking is already implemented in the model as a setter routine. If you think more of the code could be moved out of browser, I'm open to your recommendations.

Regarding the distro source package complexity, yeah I get what you mean and I agree linking to a 'source package' object directly would be a cleaner design. A source package is going to be linked to the same component regardless of the distribution, and it would be better if we could model it without referencing distros at all. However, the way source packages are implemented in launchpad is kind of wonky. There isn't really a 'source package' database table and storm object that this could be hung off of. There *is* a source_package element in the API, so you would think you could link to that, however:

<deryck> bryceh, no, it's ISourcePackage, which is poorly named. That's the package in a distro series.
<deryck> bryceh, so you're only options are to hang this on either SourcePackage (distro series) or DistributionSourcePackage (distro) and the later seems right.
 bryceh, right
 bryceh, sourcepackagename is just table magic really to enable re-use of names across distros.
 bryceh, but you can't really link to it the same way as the other objects. You could, but it's not really used the same way as the other objects.
...
<deryck> bryceh, so you've done it right. DSP is actually the distro source package, as the name suggests, [while] SP would be better named "distro series source package."

« Back to merge proposal