Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] OWSql does not save selected table/query #2659

Merged
merged 7 commits into from
Oct 17, 2017
Merged

[FIX] OWSql does not save selected table/query #2659

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6037f9e
Update owsql.py
ceprio Oct 8, 2017
8ae9a1a
Changed file according to reviewer comments and bug fix for SQL Serve…
ceprio Oct 11, 2017
a1e6411
Correction following comments from Travis CI
ceprio Oct 11, 2017
8ba7584
Cosmetic changes following reviewer's comment
ceprio Oct 14, 2017
34433be
Removed white space
ceprio Oct 14, 2017
f791857
Code change to allow exact count be made by the callee.
ceprio Oct 16, 2017
6e702a0
Removed spaces at end of lines
ceprio Oct 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 40 additions & 26 deletions Orange/widgets/data/owsql.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#!/usr/bin/python
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As widgets are mostly used from Canvas, we usually do not include this. For testing, widget can be run with python widget_module.py.

# -*- coding: utf-8 -*-
import sys
from collections import OrderedDict

Expand All @@ -20,15 +22,15 @@
MAX_DL_LIMIT = 1000000


class TableModel(PyListModel):
class TableModels(PyListModel):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one should be singular Model as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have anticipated this one.

def data(self, index, role=Qt.DisplayRole):
row = index.row()
if role == Qt.DisplayRole:
return str(self[row])
return super().data(index, role)


class BackendModel(PyListModel):
class BackendModels(PyListModel):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Model is a part of the Qt's Model/View paradigm and should remain singular.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was surprised to see that the Model was is fact a list of backends and you have to select one. Thus the 's'. I'll correct.

def data(self, index, role=Qt.DisplayRole):
row = index.row()
if role == Qt.DisplayRole:
Expand All @@ -43,7 +45,7 @@ class OWSql(OWWidget):
icon = "icons/SQLTable.svg"
priority = 30
category = "Data"
keywords = ["data", "file", "load", "read"]
keywords = ["data", "file", "load", "read", "SQL"]

class Outputs:
data = Output("Data", Table, doc="Attribute-valued data set read from the input file.")
Expand Down Expand Up @@ -85,10 +87,10 @@ def __init__(self):
vbox = gui.vBox(self.controlArea, "Server", addSpace=True)
box = gui.vBox(vbox)

self.backendmodel = BackendModel(Backend.available_backends())
self.backendmodels = BackendModels(Backend.available_backends())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, model should remain singular. If you prefer plural, I would simply use
self.backends = BackendModel(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, ok I'll do that.

self.backendcombo = QComboBox(box)
if len(self.backendmodel):
self.backendcombo.setModel(self.backendmodel)
if len(self.backendmodels):
self.backendcombo.setModel(self.backendmodels)
else:
self.Error.no_backends()
box.setEnabled(False)
Expand Down Expand Up @@ -124,17 +126,23 @@ def __init__(self):
box.layout().addWidget(self.passwordtext)

self._load_credentials()
self.tablemodels = TableModels()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tablemodel or tables, but not tablemodels


tables = gui.hBox(box)
self.tablemodel = TableModel()
self.tablecombo = QComboBox(
minimumContentsLength=35,
sizeAdjustPolicy=QComboBox.AdjustToMinimumContentsLength
)
self.tablecombo.setModel(self.tablemodel)
self.tablecombo.setModel(self.tablemodels)
self.tablecombo.setToolTip('table')
tables.layout().addWidget(self.tablecombo)
self.tablecombo.activated[int].connect(self.select_table)
self.connect()

index = self.tablecombo.findText(str(self.table))
if index != -1:
self.tablecombo.setCurrentIndex(index)
self.tablecombo.activated[int].connect(self.select_table) # set up the callback to select_table in case of selection change
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move the commend in a line before the command? We try to limit the lines in source code to maximum of 100 characters (though some go even further and limit it to 79)


self.connectbutton = gui.button(
tables, self, '↻', callback=self.connect)
self.connectbutton.setSizePolicy(
Expand Down Expand Up @@ -167,7 +175,8 @@ def __init__(self):
callback=self.open_table)

gui.rubber(self.buttonsArea)
QTimer.singleShot(0, self.connect)

QTimer.singleShot(0, self.select_table)

def _load_credentials(self):
self._parse_host_port()
Expand All @@ -182,8 +191,8 @@ def _load_credentials(self):

def _save_credentials(self):
cm = self._credential_manager(self.host, self.port)
cm.username = self.username
cm.password = self.password
cm.username = self.username or ''
cm.password = self.password or ''

def _credential_manager(self, host, port):
return CredentialManager("SQL Table: {}:{}".format(host, port))
Expand Down Expand Up @@ -217,7 +226,7 @@ def connect(self):
try:
if self.backendcombo.currentIndex() < 0:
return
backend = self.backendmodel[self.backendcombo.currentIndex()]
backend = self.backendmodels[self.backendcombo.currentIndex()]
self.backend = backend(dict(
host=self.host,
port=self.port,
Expand All @@ -232,25 +241,25 @@ def connect(self):
("Database", self.database), ("User name", self.username)
))
self.refresh_tables()
self.select_table()
except BackendError as err:
error = str(err).split('\n')[0]
self.Error.connection(error)
self.database_desc = self.data_desc_table = None
self.tablecombo.clear()

def refresh_tables(self):
self.tablemodel.clear()
self.tablemodels.clear()
self.Error.missing_extension.clear()
if self.backend is None:
self.data_desc_table = None
return

self.tablemodel.append("Select a table")
self.tablemodel.extend(self.backend.list_tables(self.schema))
self.tablemodel.append("Custom SQL")
self.tablemodels.append("Select a table")
self.tablemodels.append("Custom SQL")
self.tablemodels.extend(self.backend.list_tables(self.schema))

def select_table(self):
"Called on tablecombo selection change"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

""" is usually used for docstrings

curIdx = self.tablecombo.currentIndex()
if self.tablecombo.itemText(curIdx) != "Custom SQL":
self.custom_sql.setVisible(False)
Expand All @@ -260,6 +269,8 @@ def select_table(self):
self.data_desc_table = None
self.database_desc["Table"] = "(None)"
self.table = None
if len(str(self.sql)) > 14:
return self.open_table()

#self.Error.missing_extension(
# 's' if len(missing) > 1 else '',
Expand All @@ -272,19 +283,22 @@ def open_table(self):
self.Outputs.data.send(table)

def get_table(self):
if self.tablecombo.currentIndex() <= 0:
curIdx = self.tablecombo.currentIndex()
if curIdx <= 0:
if self.database_desc:
self.database_desc["Table"] = "(None)"
self.data_desc_table = None
return

if self.tablecombo.currentIndex() < self.tablecombo.count() - 1:
self.table = self.tablemodel[self.tablecombo.currentIndex()]
if self.tablecombo.itemText(curIdx) != "Custom SQL":
self.table = self.tablemodels[self.tablecombo.currentIndex()]
self.database_desc["Table"] = self.table
if "Query" in self.database_desc:
del self.database_desc["Query"]
what = self.table
else:
self.sql = self.table = self.sqltext.toPlainText()
what = self.sql = self.sqltext.toPlainText()
self.table = "Custom SQL"
if self.materialize:
import psycopg2
if not self.materialize_table_name:
Expand All @@ -297,11 +311,10 @@ def get_table(self):
pass
with self.backend.execute_sql_query("CREATE TABLE " +
self.materialize_table_name +
" AS " + self.table):
" AS " + self.sql):
pass
with self.backend.execute_sql_query("ANALYZE " + self.materialize_table_name):
pass
self.table = self.materialize_table_name
except (psycopg2.ProgrammingError, BackendError) as ex:
self.Error.connection(str(ex))
return
Expand All @@ -312,7 +325,7 @@ def get_table(self):
database=self.database,
user=self.username,
password=self.password),
self.table,
what,
backend=type(self.backend),
inspect_values=False)
except BackendError as ex:
Expand All @@ -321,8 +334,8 @@ def get_table(self):

self.Error.connection.clear()


sample = False

if table.approx_len() > LARGE_TABLE and self.guess_values:
confirm = QMessageBox(self)
confirm.setIcon(QMessageBox.Warning)
Expand All @@ -347,6 +360,7 @@ def get_table(self):
domain = s.get_domain(inspect_values=True)
self.Information.data_sampled()
else:
#
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove

domain = table.get_domain(inspect_values=True)
QApplication.restoreOverrideCursor()
table.domain = domain
Expand Down