Merge lp:~salgado/launchpad/remove-IDBSchema into lp:launchpad

[Guilherme Salgado]

[Guilherme Salgado (salgado)]

= Summary =

While reading the breadcrumbs code I stumbled upon IDBSchema, which I
thought was gone already. After greping around I realized it was not
used anymore, and there were a couple related functions that were being
(ab)used only in a couple places.

== Proposed fix ==

I removed the class and the two functions, fixing the call sites of the
functions. There are a bunch of tests that rely on the unusual terms
generated by said function, so I had to keep them that way instead of
using the terms provided by IEnumeratedType. I added an XXX to explain
why we need to do that.

== Tests ==

I ran lp.bugs.stories and everything pass.

== Demo and Q/A ==

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/webapp/interfaces.py
  lib/canonical/launchpad/webapp/vocabulary.py
  lib/lp/bugs/browser/bugtask.py

== Pylint notices ==

lib/lp/bugs/browser/bugtask.py
    88: [F0401] Unable to import 'lazr.uri' (No module named uri)

[Francis J. Lacoste (flacoste)]

On August 7, 2009, Guilherme Salgado wrote:
> + # XXX: salgado, 2009-08-07:
> + # We shouldn't have to build our vocabulary out of
> (item.title, + # item) tuples -- iterating over an
> EnumeratedType gives us + # ITokenizedTerms that we could use.
> However, the terms generated + # by EnumeratedType have their
> name as the token and there are + # plenty of tests that expect
> the title as the token.

Hi Salgado,

Good clean-up job.

My only gripe is that XXX should require a bug number. If this is something
that we truly want to change (and break all scripts used to using titles to
pass in status value), then file one. Otherwise, don't use an XXX there.

  status approved
  review approve

Cheers

--
Francis J. Lacoste
<email address hidden>

review: Approve

[Guilherme Salgado (salgado)]

On Fri, 2009-08-07 at 19:33 +0000, Francis J. Lacoste wrote:
> Review: Approve
> On August 7, 2009, Guilherme Salgado wrote:
> > + # XXX: salgado, 2009-08-07:
> > + # We shouldn't have to build our vocabulary out of
> > (item.title, + # item) tuples -- iterating over an
> > EnumeratedType gives us + # ITokenizedTerms that we could use.
> > However, the terms generated + # by EnumeratedType have their
> > name as the token and there are + # plenty of tests that expect
> > the title as the token.
>
>
> Hi Salgado,
>
> Good clean-up job.
>
> My only gripe is that XXX should require a bug number. If this is something
> that we truly want to change (and break all scripts used to using titles to
> pass in status value), then file one. Otherwise, don't use an XXX there.

There's no easy way of figuring out why this was done (as there's no
comment in the code), but I guess we don't want to break existing
scripts, so I've changed the comment slightly and removed the XXX.

Cheers,

--
Guilherme Salgado <email address hidden>

=== modified file 'lib/canonical/launchpad/webapp/interfaces.py'
--- lib/canonical/launchpad/webapp/interfaces.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/webapp/interfaces.py	2009-08-07 13:11:49 +0000
@@ -293,54 +293,6 @@
             (object_url_requested_for, broken_link_in_chain)
             )
 
-#
-# DBSchema
-#
-
-
-# XXX kiko 2007-02-08: this is currently unused. We need somebody to come
-# in and set up interfaces for the enums.
-class IDBSchema(Interface):
-    """A DBSchema enumeration."""
-
-    name = Attribute("Lower-cased-spaces-inserted class name of this schema.")
-
-    title = Attribute("Title of this schema.")
-
-    description = Attribute("Description of this schema.")
-
-    items = Attribute("A mapping of [name or value] -> dbschema item.")
-
-
-class IDBSchemaItem(Interface):
-    """An Item in a DBSchema enumeration."""
-
-    value = Attribute("Integer value of this enum item.")
-
-    name = Attribute("Symbolic name of this item.")
-
-    title = Attribute("Title text of this item.")
-
-    description = Attribute("Description text of this item.")
-
-    def __sqlrepr__(dbname):
-        """Return an SQL representation of this item.
-
-        The dbname attribute is required as part of the sqlobject
-        interface, but it not used in this case.
-        """
-
-    def __eq__(other):
-        """An item is equal if it is from the same DBSchema and has the same
-        value.
-        """
-
-    def __ne__(other):
-        """not __eq__"""
-
-    def __hash__():
-        """Returns a hash value."""
-
 # XXX kiko 2007-02-08: this needs reconsideration if we are to make it a truly
 # generic thing. The problem lies in the fact that half of this (user, login,
 # time zone, developer) is actually useful inside webapp/, and the other half

=== modified file 'lib/canonical/launchpad/webapp/vocabulary.py'
--- lib/canonical/launchpad/webapp/vocabulary.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/webapp/vocabulary.py	2009-08-07 13:11:49 +0000
@@ -1,6 +1,8 @@
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+# pylint: disable-msg=E0211,E0213
+
 """Vocabularies pulling stuff from the database.
 
 You probably don't want to use these classes directly - see the
@@ -14,10 +16,8 @@
     'SQLObjectVocabularyBase',
     'NamedSQLObjectVocabulary',
     'NamedSQLObjectHugeVocabulary',
-    'sortkey_ordered_vocab_factory',
     'CountableIterator',
     'BatchedCountableIterator',
-    'vocab_factory'
 ]
 
 from sqlobject import AND, CONTAINSSTRING
@@ -26,7 +26,6 @@
 from zope.schema.interfaces import IVocabulary, IVocabularyTokenized
 from zope.schema.vocabulary import SimpleTerm
 from zope.security.proxy import isinstance as zisinstance
-from zope.schema.vocabulary import SimpleVocabulary
 
 from canonical.database.sqlbase import SQLBase
 
@@ -353,39 +352,3 @@
             clause = AND(clause, self._filter)
         results = self._table.select(clause, orderBy=self._orderBy)
         return self.iterator(results.count(), results, self.toTerm)
-
-
-# TODO: Make DBSchema classes provide an interface, so we can directly
-# adapt IDBSchema to IVocabulary
-def vocab_factory(schema, noshow=None):
-    """Factory for IDBSchema -> IVocabulary adapters.
-
-    This function returns a callable object that creates vocabularies
-    from dbschemas.
-
-    The items appear in value order, lowest first.
-    """
-    if noshow is None:
-        noshow = []
-    def factory(context, schema=schema, noshow=noshow):
-        """Adapt IDBSchema to IVocabulary."""
-        items = [(item.title, item) for item in schema.items
-                 if item not in noshow]
-        return SimpleVocabulary.fromItems(items)
-    return factory
-
-def sortkey_ordered_vocab_factory(schema, noshow=None):
-    """Another factory for IDBSchema -> IVocabulary.
-
-    This function returns a callable object that creates a vocabulary
-    from a dbschema ordered by that schema's sortkey.
-    """
-    if noshow is None:
-        noshow = []
-    def factory(context, schema=schema, noshow=noshow):
-        """Adapt IDBSchema to IVocabulary."""
-        items = [(item.title, item)
-                 for item in schema.items
-                 if item not in noshow]
-        return SimpleVocabulary.fromItems(items)
-    return factory

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2009-08-04 14:57:07 +0000
+++ lib/lp/bugs/browser/bugtask.py	2009-08-07 13:11:49 +0000
@@ -133,7 +133,6 @@
 from canonical.launchpad.webapp.batching import TableBatchNavigator
 from canonical.launchpad.webapp.menu import structured
 from canonical.launchpad.webapp.tales import PersonFormatterAPI
-from canonical.launchpad.webapp.vocabulary import vocab_factory
 
 from canonical.lazr.interfaces import IObjectPrivacy
 from lazr.restful.interfaces import IJSONRequestCache
@@ -1187,12 +1186,18 @@
                 # The user has to be able to see the current value.
                 status_noshow.remove(self.context.status)
 
-            status_vocab_factory = vocab_factory(
-                BugTaskStatus, noshow=status_noshow)
+            # XXX: salgado, 2009-08-07:
+            # We shouldn't have to build our vocabulary out of (item.title,
+            # item) tuples -- iterating over an EnumeratedType gives us
+            # ITokenizedTerms that we could use. However, the terms generated
+            # by EnumeratedType have their name as the token and there are
+            # plenty of tests that expect the title as the token.
+            status_items = [
+                (item.title, item) for item in BugTaskStatus.items
+                if item not in status_noshow]
             status_field = Choice(
-                __name__='status',
-                title=self.schema['status'].title,
-                vocabulary=status_vocab_factory(self.context))
+                __name__='status', title=self.schema['status'].title,
+                vocabulary=SimpleVocabulary.fromItems(status_items))
 
             self.form_fields = self.form_fields.omit('status')
             self.form_fields += formlib.form.Fields(status_field)
@@ -3013,14 +3018,21 @@
     def status_widget_items(self):
         """The available status items as JSON."""
         if self.user is not None:
-            status_vocab_factory = vocab_factory(
-                BugTaskStatus, noshow=[BugTaskStatus.UNKNOWN])
+            # XXX: salgado, 2009-08-07:
+            # We shouldn't have to build our vocabulary out of (item.title,
+            # item) tuples -- iterating over an EnumeratedType gives us
+            # ITokenizedTerms that we could use. However, the terms generated
+            # by EnumeratedType have their name as the token and there are
+            # plenty of tests that expect the title as the token.
+            status_items = [
+                (item.title, item) for item in BugTaskStatus.items
+                if item != BugTaskStatus.UNKNOWN]
 
             disabled_items = [status for status in BugTaskStatus.items
                 if not self.context.canTransitionToStatus(status, self.user)]
 
             items = vocabulary_to_choice_edit_items(
-                status_vocab_factory(self.context),
+                SimpleVocabulary.fromItems(status_items),
                 css_class_prefix='status',
                 disabled_items=disabled_items)
         else:
@@ -3032,11 +3044,18 @@
     def importance_widget_items(self):
         """The available status items as JSON."""
         if self.user is not None:
-            importance_vocab_factory = vocab_factory(
-                BugTaskImportance, noshow=[BugTaskImportance.UNKNOWN])
+            # XXX: salgado, 2009-08-07:
+            # We shouldn't have to build our vocabulary out of (item.title,
+            # item) tuples -- iterating over an EnumeratedType gives us
+            # ITokenizedTerms that we could use. However, the terms generated
+            # by EnumeratedType have their name as the token and there are
+            # plenty of tests that expect the title as the token.
+            importance_items = [
+                (item.title, item) for item in BugTaskImportance.items
+                if item != BugTaskImportance.UNKNOWN]
 
             items = vocabulary_to_choice_edit_items(
-                importance_vocab_factory(self.context),
+                SimpleVocabulary.fromItems(importance_items),
                 css_class_prefix='importance')
         else:
             items = '[]'