launchpad-reviewers team mailing list archive

[Graham Binns <graham@xxxxxxxxxxxxx>]

Review: Needs Fixing code


Hi Colin,

Overall, I like this branch, and I'm happy for it to land (I'll just
have Robert hold you to your commitment to trim the LoC elsewhere ;)).

Just one question:

188	+    # Really IBinaryPackagePublishingHistory, patched in
189	+    # _schema_circular_imports.py.
190	+    @operation_returns_entry(Interface)
191	+    @operation_parameters(
192	+        new_component=TextLine(title=u"The new component name."),
193	+        new_section=TextLine(title=u"The new section name."),
194	+        new_priority=TextLine(title=u"The new priority name."))
195	+    @export_write_operation()
196	+    @operation_for_version("devel")
197	+    def changeOverride(new_component=None, new_section=None,
198	+                       new_priority=None):

I'm confused as to why you're using TextLine here for new_priority
(unless there's some pitfall in using copy_field() between different
interfaces; I don't know about that). You should be able to use
copy_field() here to copy the relevant field ( in this case I'm guessing
it'd be IBinaryPublishingHistory.priority), which would mean that you
don't have to do a lookup for the priority in name_priority_map later on
(line 297 of the diff)- lazr.restful would take care of that for you.

You can see an example of this in lib/lp/bugs/interfaces/bugtask.py:750.


-- 
https://code.launchpad.net/~cjwatson/launchpad/export-change-override/+merge/109549
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.