Small review fixups for SQL implementation.

Respond to the low-hanging code review fruit:

- Clean up some comments
- Remove an acidentally added duplicate init_autosave
- Combine two test_history tests
- Move test_init cleanup into a fixture to ensure it gets called.
- Name the _ argument of bind(_) to _key
- Ensure index is valid for first_item/last_item
- Move SqlException to top of module
- Rename test_index to test_getitem
- Return QItemFlags.None instead of None
- Fix copyright dates (its 2017 now!)
- Use * to force some args to be keyword-only
- Make some returns explicit
- Add sql to LOGGER_NAMES
- Add a comment to explain the sql escape statement

qutebrowser/app.py

@@ -426,7 +426,6 @@ def _init_modules(args, crash_handler):
 
     log.init.debug("Initializing keys...")
     keyconf.init(qApp)
-    save_manager.init_autosave()
 
     log.init.debug("Initializing sql...")
     sql.init()

qutebrowser/browser/urlmarks.py

@@ -104,10 +104,7 @@ class UrlMarkManager(sql.SqlTable):
 
 class QuickmarkManager(UrlMarkManager):
 
-    """Manager for quickmarks.
-
-    The primary key for quickmarks is their *name*.
-    """
+    """Manager for quickmarks."""
 
     def __init__(self, parent=None):
         super().__init__('Quickmarks', ['name', 'url'], primary_key='name',
@@ -179,7 +176,7 @@ class QuickmarkManager(UrlMarkManager):
             set_mark()
 
     def delete_by_qurl(self, url):
-        """Look up a quickmark by QUrl, returning its name."""
+        """Delete a quickmark by QUrl (as opposed to by name)."""
         qtutils.ensure_valid(url)
         urlstr = url.toString(QUrl.RemovePassword | QUrl.FullyEncoded)
         try:
@@ -204,10 +201,7 @@ class QuickmarkManager(UrlMarkManager):
 
 class BookmarkManager(UrlMarkManager):
 
-    """Manager for bookmarks.
-
-    The primary key for bookmarks is their *url*.
-    """
+    """Manager for bookmarks."""
 
     def __init__(self, parent=None):
         super().__init__('Bookmarks', ['url', 'title'], primary_key='url',

qutebrowser/completion/models/base.py

@@ -36,9 +36,10 @@ class CompletionModel(QStandardItemModel):
 
     Attributes:
         column_widths: The width percentages of the columns used in the
+                       completion view.
     """
 
-    def __init__(self, column_widths=(30, 70, 0), columns_to_filter=None,
+    def __init__(self, *, column_widths=(30, 70, 0), columns_to_filter=None,
                  delete_cur_item=None, parent=None):
         super().__init__(parent)
         self.setColumnCount(3)

qutebrowser/completion/models/miscmodels.py

@@ -130,11 +130,11 @@ def buffer():
     return model
 
 
-def bind(_):
+def bind(_key):
     """A CompletionModel filled with all bindable commands and descriptions.
 
     Args:
-        _: the key being bound.
+        _key: the key being bound.
     """
     # TODO: offer a 'Current binding' completion based on the key.
     model = base.CompletionModel(column_widths=(20, 60, 20))

qutebrowser/completion/models/sqlmodel.py

@@ -1,6 +1,6 @@
 # vim: ft=python fileencoding=utf-8 sts=4 sw=4 et:
 
-# Copyright 2016 Ryan Roden-Corrent (rcorre) <ryan@rcorre.net>
+# Copyright 2016-2017 Ryan Roden-Corrent (rcorre) <ryan@rcorre.net>
 #
 # This file is part of qutebrowser.
 #
@@ -24,12 +24,14 @@ import re
 from PyQt5.QtCore import Qt, QModelIndex, QAbstractItemModel
 from PyQt5.QtSql import QSqlQueryModel
 
-from qutebrowser.utils import log
+from qutebrowser.utils import log, qtutils
 from qutebrowser.misc import sql
 
 
 class _SqlCompletionCategory(QSqlQueryModel):
-    def __init__(self, name, sort_by, sort_order, limit, select, where,
+    """Wraps a SqlQuery for use as a completion category."""
+
+    def __init__(self, name, *, sort_by, sort_order, limit, select, where,
                  columns_to_filter, parent=None):
         super().__init__(parent=parent)
         self.tablename = name
@@ -38,6 +40,8 @@ class _SqlCompletionCategory(QSqlQueryModel):
         self._fields = [query.record().fieldName(i) for i in columns_to_filter]
 
         querystr = 'select {} from {} where ('.format(select, name)
+        # the incoming pattern will have literal % and _ escaped with '\'
+        # we need to tell sql to treat '\' as an escape character
         querystr += ' or '.join("{} like ? escape '\\'".format(f)
                                 for f in self._fields)
         querystr += ')'
@@ -45,6 +49,7 @@ class _SqlCompletionCategory(QSqlQueryModel):
             querystr += ' and ' + where
 
         if sort_by:
+            assert sort_order is not None
             sortstr = 'asc' if sort_order == Qt.AscendingOrder else 'desc'
             querystr += ' order by {} {}'.format(sort_by, sortstr)
 
@@ -69,19 +74,15 @@ class SqlCompletionModel(QAbstractItemModel):
     Top level indices represent categories, each of which is backed by a single
     table. Child indices represent rows of those tables.
 
-    Class Attributes:
-        COLUMN_WIDTHS: The width percentages of the columns used in the
-                       completion view.
-
     Attributes:
         column_widths: The width percentages of the columns used in the
-                        completion view.
+                       completion view.
         columns_to_filter: A list of indices of columns to apply the filter to.
         pattern: Current filter pattern, used for highlighting.
         _categories: The category tables.
     """
 
-    def __init__(self, column_widths=(30, 70, 0), columns_to_filter=None,
+    def __init__(self, *, column_widths=(30, 70, 0), columns_to_filter=None,
                  delete_cur_item=None, parent=None):
         super().__init__(parent)
         self.columns_to_filter = columns_to_filter or [0]
@@ -103,8 +104,9 @@ class SqlCompletionModel(QAbstractItemModel):
         # categories have an empty internalPointer
         if index.isValid() and not index.internalPointer():
             return self._categories[index.row()]
+        return None
 
-    def new_category(self, name, select='*', where=None, sort_by=None,
+    def new_category(self, name, *, select='*', where=None, sort_by=None,
                      sort_order=None, limit=None):
         """Create a new completion category and add it to this model.
 
@@ -135,7 +137,7 @@ class SqlCompletionModel(QAbstractItemModel):
         Return: The item data, or None on an invalid index.
         """
         if not index.isValid() or role != Qt.DisplayRole:
-            return
+            return None
         if not index.parent().isValid():
             if index.column() == 0:
                 return self._categories[index.row()].tablename
@@ -152,7 +154,7 @@ class SqlCompletionModel(QAbstractItemModel):
         Return: The item flags, or Qt.NoItemFlags on error.
         """
         if not index.isValid():
-            return
+            return Qt.NoItemFlags
         if index.parent().isValid():
             # item
             return (Qt.ItemIsEnabled | Qt.ItemIsSelectable |
@@ -253,7 +255,9 @@ class SqlCompletionModel(QAbstractItemModel):
         for row, table in enumerate(self._categories):
             if table.rowCount() > 0:
                 parent = self.index(row, 0)
-                return self.index(0, 0, parent)
+                index = self.index(0, 0, parent)
+                qtutils.ensure_valid(index)
+                return index
         return QModelIndex()
 
     def last_item(self):
@@ -262,7 +266,9 @@ class SqlCompletionModel(QAbstractItemModel):
             childcount = table.rowCount()
             if childcount > 0:
                 parent = self.index(row, 0)
-                return self.index(childcount - 1, 0, parent)
+                index = self.index(childcount - 1, 0, parent)
+                qtutils.ensure_valid(index)
+                return index
         return QModelIndex()
 
 

qutebrowser/misc/sql.py

@@ -1,6 +1,6 @@
 # vim: ft=python fileencoding=utf-8 sts=4 sw=4 et:
 
-# Copyright 2016 Ryan Roden-Corrent (rcorre) <ryan@rcorre.net>
+# Copyright 2016-2017 Ryan Roden-Corrent (rcorre) <ryan@rcorre.net>
 #
 # This file is part of qutebrowser.
 #
@@ -27,13 +27,21 @@ from qutebrowser.utils import log
 import collections
 
 
+class SqlException(Exception):
+
+    """Raised on an error interacting with the SQL database."""
+
+    pass
+
+
 def init():
     """Initialize the SQL database connection."""
     database = QSqlDatabase.addDatabase('QSQLITE')
     # In-memory database, see https://sqlite.org/inmemorydb.html
     database.setDatabaseName(':memory:')
     if not database.open():
-        raise SqlException("Failed to open in-memory sqlite database")
+        raise SqlException("Failed to open in-memory sqlite database: {}"
+                           .format(database.lastError().text()))
 
 
 def close():
@@ -169,10 +177,3 @@ class SqlTable(QObject):
         """Remove all row from the table."""
         run_query("DELETE FROM {}".format(self._name))
         self.changed.emit()
-
-
-class SqlException(Exception):
-
-    """Raised on an error interacting with the SQL database."""
-
-    pass

qutebrowser/utils/log.py

@@ -94,7 +94,7 @@ LOGGER_NAMES = [
     'commands', 'signals', 'downloads',
     'js', 'qt', 'rfc6266', 'ipc', 'shlexer',
     'save', 'message', 'config', 'sessions',
-    'webelem', 'prompt', 'network'
+    'webelem', 'prompt', 'network', 'sql'
 ]
 
 

tests/helpers/fixtures.py

@@ -457,7 +457,7 @@ def short_tmpdir():
         yield py.path.local(tdir)  # pylint: disable=no-member
 
 
-@pytest.fixture()
+@pytest.fixture
 def init_sql():
     """Initialize the SQL module, and shut it down after the test."""
     sql.init()

tests/unit/browser/webkit/test_history.py

@@ -193,21 +193,15 @@ def test_clear(qtbot, tmpdir):
     assert lines == ['67890 http://www.the-compiler.org/']
 
 
-def test_add_item(qtbot, hist):
+@pytest.mark.parametrize('item', [
+    ('http://www.example.com', 12346, 'the title', False),
+    ('http://www.example.com', 12346, 'the title', True)
+])
+def test_add_item(qtbot, hist, item):
     list(hist.async_read())
-    url = 'http://www.example.com/'
-
-    hist.add_url(QUrl(url), atime=12345, title="the title")
-
-    assert hist[url] == (url, 'the title', 12345, False)
-
-
-def test_add_item_redirect(qtbot, hist):
-    list(hist.async_read())
-    url = 'http://www.example.com/'
-    hist.add_url(QUrl(url), redirect=True, atime=12345)
-
-    assert hist[url] == (url, '', 12345, True)
+    (url, atime, title, redirect) = item
+    hist.add_url(QUrl(url), atime=atime, title=title, redirect=redirect)
+    assert hist[url] == (url, title, atime, redirect)
 
 
 def test_add_item_redirect_update(qtbot, tmpdir, fake_save_manager):
@@ -329,9 +323,23 @@ def test_history_interface(qtbot, webview, hist_interface):
         webview.load(url)
 
 
+@pytest.fixture
+def cleanup_init():
+    yield
+    # prevent test_init from leaking state
+    hist = objreg.get('web-history')
+    hist.setParent(None)
+    objreg.delete('web-history')
+    try:
+        from PyQt5.QtWebKit import QWebHistoryInterface
+        QWebHistoryInterface.setDefaultInterface(None)
+    except:
+        pass
+
+
 @pytest.mark.parametrize('backend', [usertypes.Backend.QtWebEngine,
                                      usertypes.Backend.QtWebKit])
-def test_init(backend, qapp, tmpdir, monkeypatch):
+def test_init(backend, qapp, tmpdir, monkeypatch, cleanup_init):
     if backend == usertypes.Backend.QtWebKit:
         pytest.importorskip('PyQt5.QtWebKitWidgets')
     else:
@@ -360,9 +368,3 @@ def test_init(backend, qapp, tmpdir, monkeypatch):
         # For this to work, nothing can ever have called setDefaultInterface
         # before (so we need to test webengine before webkit)
         assert default_interface is None
-
-    # prevent interference with future tests
-    objreg.delete('web-history')
-    hist.setParent(None)
-    if backend == usertypes.Backend.QtWebKit:
-        QWebHistoryInterface.setDefaultInterface(None)

tests/unit/completion/test_sqlmodel.py

@@ -1,6 +1,6 @@
 # vim: ft=python fileencoding=utf-8 sts=4 sw=4 et:
 
-# Copyright 2016 Ryan Roden-Corrent (rcorre) <ryan@rcorre.net>
+# Copyright 2016-2017 Ryan Roden-Corrent (rcorre) <ryan@rcorre.net>
 #
 # This file is part of qutebrowser.
 #

tests/unit/misc/test_sql.py

@@ -1,6 +1,6 @@
 # vim: ft=python fileencoding=utf-8 sts=4 sw=4 et:
 
-# Copyright 2016 Ryan Roden-Corrent (rcorre) <ryan@rcorre.net>
+# Copyright 2016-2017 Ryan Roden-Corrent (rcorre) <ryan@rcorre.net>
 #
 # This file is part of qutebrowser.
 #
@@ -102,7 +102,7 @@ def test_contains():
     assert 'thirteen' in table
 
 
-def test_index():
+def test_getitem():
     table = sql.SqlTable('Foo', ['name', 'val', 'lucky'], primary_key='name')
     table.insert(['one', 1, False])
     table.insert(['nine', 9, False])