From 56f3b3a02709736ba702b3b3ef4342e3a4c98a42 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Tue, 21 Feb 2017 08:27:52 -0500 Subject: [PATCH] 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 | 1 - qutebrowser/browser/urlmarks.py | 12 ++---- qutebrowser/completion/models/base.py | 3 +- qutebrowser/completion/models/miscmodels.py | 4 +- qutebrowser/completion/models/sqlmodel.py | 34 +++++++++------- qutebrowser/misc/sql.py | 19 ++++----- qutebrowser/utils/log.py | 2 +- tests/helpers/fixtures.py | 2 +- tests/unit/browser/webkit/test_history.py | 44 +++++++++++---------- tests/unit/completion/test_sqlmodel.py | 2 +- tests/unit/misc/test_sql.py | 4 +- 11 files changed, 65 insertions(+), 62 deletions(-) diff --git a/qutebrowser/app.py b/qutebrowser/app.py index 44178bae3..33726b058 100644 --- a/qutebrowser/app.py +++ b/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() diff --git a/qutebrowser/browser/urlmarks.py b/qutebrowser/browser/urlmarks.py index 5f7a4fb77..580d77881 100644 --- a/qutebrowser/browser/urlmarks.py +++ b/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', diff --git a/qutebrowser/completion/models/base.py b/qutebrowser/completion/models/base.py index 112c6ed00..f5148ff9c 100644 --- a/qutebrowser/completion/models/base.py +++ b/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) diff --git a/qutebrowser/completion/models/miscmodels.py b/qutebrowser/completion/models/miscmodels.py index 8bb234521..f5ed72f5d 100644 --- a/qutebrowser/completion/models/miscmodels.py +++ b/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)) diff --git a/qutebrowser/completion/models/sqlmodel.py b/qutebrowser/completion/models/sqlmodel.py index 2c7a25bc0..1053fb2f2 100644 --- a/qutebrowser/completion/models/sqlmodel.py +++ b/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) +# Copyright 2016-2017 Ryan Roden-Corrent (rcorre) # # 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() diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py index e0b55dbd2..c9249d362 100644 --- a/qutebrowser/misc/sql.py +++ b/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) +# Copyright 2016-2017 Ryan Roden-Corrent (rcorre) # # 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 diff --git a/qutebrowser/utils/log.py b/qutebrowser/utils/log.py index 513c05062..6cdc61f41 100644 --- a/qutebrowser/utils/log.py +++ b/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' ] diff --git a/tests/helpers/fixtures.py b/tests/helpers/fixtures.py index 7fc05a578..0b69ad89f 100644 --- a/tests/helpers/fixtures.py +++ b/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() diff --git a/tests/unit/browser/webkit/test_history.py b/tests/unit/browser/webkit/test_history.py index 890ff9a89..9d8711152 100644 --- a/tests/unit/browser/webkit/test_history.py +++ b/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) diff --git a/tests/unit/completion/test_sqlmodel.py b/tests/unit/completion/test_sqlmodel.py index f6daa722c..044eecb35 100644 --- a/tests/unit/completion/test_sqlmodel.py +++ b/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) +# Copyright 2016-2017 Ryan Roden-Corrent (rcorre) # # This file is part of qutebrowser. # diff --git a/tests/unit/misc/test_sql.py b/tests/unit/misc/test_sql.py index 8bf7ddb4b..f4898b25d 100644 --- a/tests/unit/misc/test_sql.py +++ b/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) +# Copyright 2016-2017 Ryan Roden-Corrent (rcorre) # # 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])