Launchpad itself

Code review comment for ~ilasc/launchpad:bug-1818755

Git
lp:~ilasc/launchpad
bug-1818755
Merge into master

Revision history for this message

Colin Watson (cjwatson) wrote on 2019-11-15:

So on reflection I think the attempt to use a non-DB-backed vocabulary for this was a serious red herring that I should have steered you away from sooner. I thought it was more of a brief temporary experiment, but it's led you down the path of making quite time-consuming changes all over the place to support it that aren't going to be mergeable. Similarly, hardcoding IDs is never going to be the right thing to do and I think relying on them has perhaps got in the way of better approaches. I hope these suggestions get you back on the right path.

Regarding how to do the presentational side of these changes, particularly the parts in SnapAddView and SnapEditView: as I mentioned before, there are a couple of possible approaches. We do IMO want to keep the approach where store_series and distro_series are only ever edited in combined form in the UI, even if the store_series part of that is mostly vestigial. That means that we need to figure out a way to represent those combinations in a way that's less confusing.

At the moment, store_distro_series is presented using a LaunchpadRadioWidget, which just renders as a list of radio buttons with choices from the vocabulary. That's fine - there doesn't seem to be a particular need to change that, so leave it alone. The vocabulary's terms are "titled": that is, as well as having a value (a SnappyDistroSeries object) and a token (a machine-readable identifier which can be used in HTML forms), they also have a title which is intended for display to humans. The title seems like the right thing to change in this case.

Now, a vocabulary can either make up its own titles, or it can delegate that to something else. If you look at SnappyDistroSeriesVocabulary.toTerm, it returns SimpleTerm(obj, token, obj.title), meaning that it's just using the "title" property of SnappyDistroSeries. That does mean we're leaking presentational details down into the model layer a bit, but that's relatively harmless in this case as IIRC that title isn't used by anything else. So we probably might as well just decide on what each of the choices should look like in the UI and then make SnappyDistroSeries.title return those.

So on reflection I think the attempt to use a non-DB-backed vocabulary for this was a serious red herring that I should have steered you away from sooner.  I thought it was more of a brief temporary experiment, but it's led you down the path of making quite time-consuming changes all over the place to support it that aren't going to be mergeable.  Similarly, hardcoding IDs is never going to be the right thing to do and I think relying on them has perhaps got in the way of better approaches.  I hope these suggestions get you back on the right path.

Regarding how to do the presentational side of these changes, particularly the parts in SnapAddView and SnapEditView: as I mentioned before, there are a couple of possible approaches.  We do IMO want to keep the approach where store_series and distro_series are only ever edited in combined form in the UI, even if the store_series part of that is mostly vestigial.  That means that we need to figure out a way to represent those combinations in a way that's less confusing.

At the moment, store_distro_series is presented using a LaunchpadRadioWidget, which just renders as a list of radio buttons with choices from the vocabulary.  That's fine - there doesn't seem to be a particular need to change that, so leave it alone.  The vocabulary's terms are "titled": that is, as well as having a value (a SnappyDistroSeries object) and a token (a machine-readable identifier which can be used in HTML forms), they also have a title which is intended for display to humans.  The title seems like the right thing to change in this case.

Now, a vocabulary can either make up its own titles, or it can delegate that to something else.  If you look at SnappyDistroSeriesVocabulary.toTerm, it returns SimpleTerm(obj, token, obj.title), meaning that it's just using the "title" property of SnappyDistroSeries.  That does mean we're leaking presentational details down into the model layer a bit, but that's relatively harmless in this case as IIRC that title isn't used by anything else.  So we probably might as well just decide on what each of the choices should look like in the UI and then make SnappyDistroSeries.title return those.

« Back to merge proposal