From 2435a20bcd13292e0e8800dcd67cbf31f0bf8b90 Mon Sep 17 00:00:00 2001 From: Eduardo Blancas Date: Fri, 21 Apr 2023 19:03:33 -0600 Subject: [PATCH 01/21] adds display module --- src/sql/command.py | 9 +++++++++ src/sql/display.py | 21 +++++++++++++++++++++ src/sql/magic.py | 7 +++++-- 3 files changed, 35 insertions(+), 2 deletions(-) create mode 100644 src/sql/display.py diff --git a/src/sql/command.py b/src/sql/command.py index c63307591..4da0373ab 100644 --- a/src/sql/command.py +++ b/src/sql/command.py @@ -20,6 +20,9 @@ class SQLCommand: """ def __init__(self, magic, user_ns, line, cell) -> None: + self._line = line + self._cell = cell + self.args = parse.magic_args(magic.execute, line) # self.args.line (everything that appears after %sql/%%sql in the first line) # is splited in tokens (delimited by spaces), this checks if we have one arg @@ -97,3 +100,9 @@ def result_var(self): def _var_expand(self, sql, user_ns, magic): return Template(sql).render(user_ns) + + def __repr__(self) -> str: + return ( + f"{type(self).__name__}(line={self._line!r}, cell={self._cell!r}) -> " + f"({self.sql!r}, {self.sql_original!r})" + ) diff --git a/src/sql/display.py b/src/sql/display.py new file mode 100644 index 000000000..9a3bdf6c6 --- /dev/null +++ b/src/sql/display.py @@ -0,0 +1,21 @@ +from IPython.display import display + + +class Message: + def __init__(self, message, style=None) -> None: + self._message = message + self._style = "" or style + + def _repr_html_(self): + return f'{self._message}' + + def __repr__(self) -> str: + return self._message + + +def message(message): + display(Message(message)) + + +def message_success(message): + display(Message(message, style="color: green")) diff --git a/src/sql/magic.py b/src/sql/magic.py index 27b051847..a47f4cfeb 100644 --- a/src/sql/magic.py +++ b/src/sql/magic.py @@ -21,6 +21,7 @@ import sql.connection import sql.parse import sql.run +from sql import display from sql.store import store from sql.command import SQLCommand from sql.magic_plot import SqlPlotMagic @@ -344,6 +345,7 @@ def interactive_execute_wrapper(**kwargs): alias=args.alias, ) payload["connection_info"] = conn._get_curr_sqlalchemy_connection_info() + if args.persist: return self._persist_dataframe( command.sql, conn, user_ns, append=False, index=not args.no_index @@ -369,7 +371,7 @@ def interactive_execute_wrapper(**kwargs): self._store.store(args.save, command.sql_original, with_=args.with_) if args.no_execute: - print("Skipping execution...") + display.message("Skipping execution...") return try: @@ -441,7 +443,8 @@ def _persist_dataframe(self, raw, conn, user_ns, append=False, index=True): if_exists = "append" if append else "fail" frame.to_sql(table_name, conn.session.engine, if_exists=if_exists, index=index) - return "Persisted %s" % table_name + + display.message_success(f"Success! Persisted {table_name} to the database.") def load_ipython_extension(ip): From bfd609124fb613acb544403f1559f3bbe394b399 Mon Sep 17 00:00:00 2001 From: Eduardo Blancas Date: Fri, 21 Apr 2023 19:10:54 -0600 Subject: [PATCH 02/21] displaying connections in a table [wip] --- src/sql/connection.py | 7 +++++-- src/sql/display.py | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/sql/connection.py b/src/sql/connection.py index ae8468c04..88eff0a79 100644 --- a/src/sql/connection.py +++ b/src/sql/connection.py @@ -318,6 +318,7 @@ def assign_name(cls, engine): def connection_list(cls): """Returns the list of connections, appending '*' to the current one""" result = [] + for key in sorted(cls.connections): conn = cls.connections[key] @@ -333,9 +334,11 @@ def connection_list(cls): else: repr_ = f"{prefix} {engine_url!r}" - result.append(repr_) + result.append([repr_]) + + from sql.display import Table - return "\n".join(result) + return Table(headers=["Connection name"], rows=result) @classmethod def close(cls, descriptor): diff --git a/src/sql/display.py b/src/sql/display.py index 9a3bdf6c6..36f9bc717 100644 --- a/src/sql/display.py +++ b/src/sql/display.py @@ -1,6 +1,25 @@ +from prettytable import PrettyTable from IPython.display import display +class Table: + def __init__(self, headers, rows) -> None: + self._table = PrettyTable() + self._table.field_names = headers + + for row in rows: + self._table.add_row(row) + + self._table_html = self._table.get_html_string() + self._table_txt = self._table.get_string() + + def __repr__(self) -> str: + return self._table_txt + + def _repr_html_(self) -> str: + return self._table_html + + class Message: def __init__(self, message, style=None) -> None: self._message = message From 83e529d147776f69ba5660649719b39a43c28484 Mon Sep 17 00:00:00 2001 From: Eduardo Blancas Date: Sat, 27 May 2023 13:56:57 -0600 Subject: [PATCH 03/21] fixes magic-sql.md --- doc/api/magic-sql.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/api/magic-sql.md b/doc/api/magic-sql.md index 3228e2793..82b04c0ff 100644 --- a/doc/api/magic-sql.md +++ b/doc/api/magic-sql.md @@ -111,7 +111,7 @@ To make all subsequent queries to use certain connection, pass the connection na You can inspect which is the current active connection: ```{code-cell} ipython3 -%sql --list +%sql --connections ``` For more details on managing connections, see [Switch connections](../howto.md#switch-connections). @@ -121,7 +121,7 @@ For more details on managing connections, see [Switch connections](../howto.md#s ## List connections ```{code-cell} ipython3 -%sql --list +%sql --connections ``` ## Close connection From eb2fb8f7bfa945d9ac570f44778e618581ca4b39 Mon Sep 17 00:00:00 2001 From: Eduardo Blancas Date: Sat, 27 May 2023 13:57:05 -0600 Subject: [PATCH 04/21] documents display module --- doc/community/developer-guide.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/doc/community/developer-guide.md b/doc/community/developer-guide.md index c5ee89030..0aa6672d4 100644 --- a/doc/community/developer-guide.md +++ b/doc/community/developer-guide.md @@ -23,6 +23,22 @@ Before continuing, ensure you have a working [development environment.](https:// +++ +## Displaying messages + +You can use `message` (contextual information) and `message_success` (successful operations) to show feedback to the user. Here's an example: + +```{code-cell} ipython3 +from sql.display import message, message_success +``` + +```{code-cell} ipython3 +message("Some information") +``` + +```{code-cell} ipython3 +message_success("Some operation finished successfully!") +``` + ## Throwing errors When writing Python libraries, we often throw errors (and display error tracebacks) to let users know that something went wrong. However, JupySQL is an abstraction for executing SQL queries; hence, Python tracebacks a useless to end-users since they expose JupySQL's internals. From 5901dda490c93c4d2e05430d45cd0f2449ae22c7 Mon Sep 17 00:00:00 2001 From: Eduardo Blancas Date: Sat, 27 May 2023 13:59:31 -0600 Subject: [PATCH 05/21] display improvements --- src/sql/connection.py | 53 ++++++++++++++++++++++++++---------- src/sql/display.py | 13 ++++++++- src/sql/magic.py | 2 +- src/sql/run.py | 13 ++++----- src/tests/test_connection.py | 32 +++++++++++++++++++++- src/tests/test_run.py | 14 +++++----- 6 files changed, 95 insertions(+), 32 deletions(-) diff --git a/src/sql/connection.py b/src/sql/connection.py index 56798989f..fd114ee21 100644 --- a/src/sql/connection.py +++ b/src/sql/connection.py @@ -10,7 +10,7 @@ from sql.store import store from sql.telemetry import telemetry -from sql import exceptions +from sql import exceptions, display from sql.error_message import detail from ploomber_core.exceptions import modify_exceptions @@ -339,8 +339,7 @@ def set(cls, descriptor, displaycon, connect_args=None, creator=None, alias=None else: if cls.connections: if displaycon: - # display list of connections - print(cls.connection_list()) + cls.display_current_connection() elif os.getenv("DATABASE_URL"): cls.current = Connection.from_connect_str( connect_str=os.getenv("DATABASE_URL"), @@ -359,9 +358,11 @@ def assign_name(cls, engine): return name @classmethod - def connection_list(cls): - """Returns the list of connections, appending '*' to the current one""" - result = [] + def _get_connections(cls): + """ + Return a list of dictionaries with url (str), current (bool), and alias (str) + """ + connections = [] for key in sorted(cls.connections): conn = cls.connections[key] @@ -371,18 +372,42 @@ def connection_list(cls): else: engine_url = conn.metadata.bind.url if IS_SQLALCHEMY_ONE else conn.url - prefix = "* " if conn == cls.current else " " + current = conn == cls.current - if conn.alias: - repr_ = f"{prefix} ({conn.alias}) {engine_url!r}" - else: - repr_ = f"{prefix} {engine_url!r}" + connections.append( + { + "current": current, + "url": str(engine_url), + "alias": conn.alias, + } + ) - result.append([repr_]) + return connections - from sql.display import Table + @classmethod + def display_current_connection(cls): + for conn in cls._get_connections(): + if conn["current"]: + alias = conn.get("alias") + if alias: + display.message(f"Running query in {alias!r}") + else: + display.message(f"Running query in {conn['url']!r}") - return Table(headers=["Connection name"], rows=result) + @classmethod + def connections_table(cls): + """Returns the current connections as a table""" + connections = cls._get_connections() + + def map_values(d): + d["current"] = "*" if d["current"] else "" + d["alias"] = d["alias"] if d["alias"] else "" + return d + + return display.Table( + headers=["current", "url", "alias"], + rows=[list(map_values(c).values()) for c in connections], + ) @classmethod def close(cls, descriptor): diff --git a/src/sql/display.py b/src/sql/display.py index 36f9bc717..26de4f008 100644 --- a/src/sql/display.py +++ b/src/sql/display.py @@ -1,9 +1,16 @@ +""" +A module to display confirmation messages and contextual information to the user +""" from prettytable import PrettyTable from IPython.display import display class Table: + """Represents a table""" + def __init__(self, headers, rows) -> None: + self._headers = headers + self._rows = rows self._table = PrettyTable() self._table.field_names = headers @@ -21,20 +28,24 @@ def _repr_html_(self) -> str: class Message: + """Message for the user""" + def __init__(self, message, style=None) -> None: self._message = message self._style = "" or style def _repr_html_(self): - return f'{self._message}' + return f'{self._message}' def __repr__(self) -> str: return self._message def message(message): + """Display a generic message""" display(Message(message)) def message_success(message): + """Display a success message""" display(Message(message, style="color: green")) diff --git a/src/sql/magic.py b/src/sql/magic.py index a79a9eda4..08dab108a 100644 --- a/src/sql/magic.py +++ b/src/sql/magic.py @@ -335,7 +335,7 @@ def interactive_execute_wrapper(**kwargs): interact(interactive_execute_wrapper, **interactive_dict) return if args.connections: - return sql.connection.Connection.connections + return sql.connection.Connection.connections_table() elif args.close: return sql.connection.Connection.close(args.close) diff --git a/src/sql/run.py b/src/sql/run.py index 301bb2361..b9f0e9436 100644 --- a/src/sql/run.py +++ b/src/sql/run.py @@ -11,7 +11,7 @@ import sqlalchemy import sqlparse from sql.connection import Connection -from sql import exceptions +from sql import exceptions, display from .column_guesser import ColumnGuesserMixin try: @@ -365,12 +365,9 @@ def csv(self, filename=None, **format_params): return outfile.getvalue() -def interpret_rowcount(rowcount): - if rowcount < 0: - result = "Done." - else: - result = "%d rows affected." % rowcount - return result +def display_affected_rowcount(rowcount): + if rowcount > 0: + display.message_success(f"{rowcount} rows affected.") class FakeResultProxy(object): @@ -551,7 +548,7 @@ def run(conn, sql, config): if result and config.feedback: if hasattr(result, "rowcount"): - print(interpret_rowcount(result.rowcount)) + display_affected_rowcount(result.rowcount) # bypass ResultSet and use duckdb's native method to return a pandas data frame if duckdb_autopandas: diff --git a/src/tests/test_connection.py b/src/tests/test_connection.py index 50f9bb7c7..aead6843e 100644 --- a/src/tests/test_connection.py +++ b/src/tests/test_connection.py @@ -32,7 +32,10 @@ def mock_postgres(monkeypatch, cleanup): def test_password_isnt_displayed(mock_postgres): Connection.from_connect_str("postgresql://user:topsecret@somedomain.com/db") - assert "topsecret" not in Connection.connection_list() + table = Connection.connections_table() + + assert "topsecret" not in str(table) + assert "topsecret" not in table._repr_html_() def test_connection_name(mock_postgres): @@ -266,3 +269,30 @@ def test_no_current_connection_and_get_info(monkeypatch, mock_database): monkeypatch.setattr(conn, "session", None) assert conn._get_curr_sqlalchemy_connection_info() is None + + +def test_get_connections(): + Connection(engine=create_engine("sqlite://")) + Connection(engine=create_engine("duckdb://")) + + assert Connection._get_connections() == [ + {"url": "duckdb://", "current": True, "alias": None}, + {"url": "sqlite://", "current": False, "alias": None}, + ] + + +def test_display_current_connection(capsys): + Connection(engine=create_engine("duckdb://")) + Connection.display_current_connection() + + captured = capsys.readouterr() + assert captured.out == "Running query in 'duckdb://'\n" + + +def test_connections_table(): + Connection(engine=create_engine("sqlite://")) + Connection(engine=create_engine("duckdb://")) + + connections = Connection.connections_table() + assert connections._headers == ["current", "url", "alias"] + assert connections._rows == [["*", "duckdb://", ""], ["", "sqlite://", ""]] diff --git a/src/tests/test_run.py b/src/tests/test_run.py index c22532946..2c8d113a2 100644 --- a/src/tests/test_run.py +++ b/src/tests/test_run.py @@ -15,7 +15,7 @@ is_postgres_or_redshift, select_df_type, set_autocommit, - interpret_rowcount, + display_affected_rowcount, ) @@ -143,19 +143,19 @@ def test_sql_is_empty(mock_conns, mock_config): def test_run(monkeypatch, mock_conns, mock_resultset, config_pandas): monkeypatch.setattr("sql.run.handle_postgres_special", Mock()) monkeypatch.setattr("sql.run._commit", Mock()) - monkeypatch.setattr("sql.run.interpret_rowcount", Mock()) + monkeypatch.setattr("sql.run.display_affected_rowcount", Mock()) monkeypatch.setattr("sql.run.ResultSet", mock_resultset) output = run(mock_conns, "\\", config_pandas) assert isinstance(output, type(mock_resultset.DataFrame())) -def test_interpret_rowcount(): - assert interpret_rowcount(-1) == "Done." - assert interpret_rowcount(1) == "%d rows affected." % 1 +def test_display_affected_rowcount(): + assert display_affected_rowcount(-1) == "Done." + assert display_affected_rowcount(1) == "%d rows affected." % 1 -def test__commit_is_called( +def test_commit_is_called( monkeypatch, mock_conns, mock_config, @@ -163,7 +163,7 @@ def test__commit_is_called( mock__commit = Mock() monkeypatch.setattr("sql.run._commit", mock__commit) monkeypatch.setattr("sql.run.handle_postgres_special", Mock()) - monkeypatch.setattr("sql.run.interpret_rowcount", Mock()) + monkeypatch.setattr("sql.run.display_affected_rowcount", Mock()) monkeypatch.setattr("sql.run.ResultSet", Mock()) run(mock_conns, "\\", mock_config) From da3cb8cdb1bdf0e9ca10ee230cbdbdcc232ee33c Mon Sep 17 00:00:00 2001 From: Eduardo Blancas Date: Sat, 27 May 2023 17:17:25 -0600 Subject: [PATCH 06/21] fix for backwards compat --- src/sql/connection.py | 8 +++++--- src/sql/display.py | 32 ++++++++++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/src/sql/connection.py b/src/sql/connection.py index fd114ee21..2506cfb10 100644 --- a/src/sql/connection.py +++ b/src/sql/connection.py @@ -360,7 +360,7 @@ def assign_name(cls, engine): @classmethod def _get_connections(cls): """ - Return a list of dictionaries with url (str), current (bool), and alias (str) + Return a list of dictionaries """ connections = [] @@ -377,8 +377,10 @@ def _get_connections(cls): connections.append( { "current": current, + "key": key, "url": str(engine_url), "alias": conn.alias, + "connection": conn, } ) @@ -404,9 +406,9 @@ def map_values(d): d["alias"] = d["alias"] if d["alias"] else "" return d - return display.Table( + return display.ConnectionsTable( headers=["current", "url", "alias"], - rows=[list(map_values(c).values()) for c in connections], + rows_maps=[map_values(c) for c in connections], ) @classmethod diff --git a/src/sql/display.py b/src/sql/display.py index 26de4f008..436eed46d 100644 --- a/src/sql/display.py +++ b/src/sql/display.py @@ -8,6 +8,8 @@ class Table: """Represents a table""" + FOOTER = "" + def __init__(self, headers, rows) -> None: self._headers = headers self._rows = rows @@ -21,10 +23,36 @@ def __init__(self, headers, rows) -> None: self._table_txt = self._table.get_string() def __repr__(self) -> str: - return self._table_txt + return self._table_txt + "\n" + self.FOOTER def _repr_html_(self) -> str: - return self._table_html + return self._table_html + "\n" + self.FOOTER + + +class ConnectionsTable(Table): + FOOTER = "Active connections" + + def __init__(self, headers, rows_maps) -> None: + self._rows_maps = rows_maps + + def get_values(d): + d = d.copy() + del d["connection"] + del d["key"] + return list(d.values()) + + rows = [get_values(r) for r in rows_maps] + super().__init__(headers=headers, rows=rows) + + def __getitem__(self, key: str): + """ + This method is provided for backwards compatibility. Before + creating ConnectionsTable, `%sql --connections` returned a dictionary, + hence users could retrieve connections using __getitem__ + """ + for row in self._rows_maps: + if row["key"] == key: + return row["connection"] class Message: From 1ff47f0d0ae476f13fa1bff8982fe321d4ad2ad8 Mon Sep 17 00:00:00 2001 From: Eduardo Blancas Date: Sat, 27 May 2023 17:41:31 -0600 Subject: [PATCH 07/21] example on how to hide connection string using --alias --- CHANGELOG.md | 1 + doc/howto.md | 23 +++++++++++++++++++++++ src/sql/display.py | 11 ++++++----- 3 files changed, 30 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ec53e7ce..007579c60 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ * [Feature] Support for printing capture variables using `=<<` syntax (by [@jorisroovers](https://github.com/jorisroovers)) * [Feature] Adds `--persist-replace` argument to replace existing tables when persisting data frames (#440) +* [Doc] Hiding connection string when passing `--alias` when opening a connection ## 0.7.5 (2023-05-24) diff --git a/doc/howto.md b/doc/howto.md index 94ed08452..c548fa90f 100644 --- a/doc/howto.md +++ b/doc/howto.md @@ -389,3 +389,26 @@ import warnings warnings.filterwarnings("ignore", category=FutureWarning) ``` + +```{code-cell} ipython3 +conns = %sql --connections +conns["db-three"] +``` + +## Hide connection string + +If you want to hide the connection string, pass an alias + +```{code-cell} ipython3 +%sql --close duckdb:// +``` + +```{code-cell} ipython3 +%sql duckdb:// --alias myconnection +``` + +The alias will be displayed instead of the connection string: + +```{code-cell} ipython3 +%sql SELECT * FROM 'penguins.csv' LIMIT 3 +``` diff --git a/src/sql/display.py b/src/sql/display.py index 436eed46d..53df79e71 100644 --- a/src/sql/display.py +++ b/src/sql/display.py @@ -8,7 +8,7 @@ class Table: """Represents a table""" - FOOTER = "" + TITLE = "" def __init__(self, headers, rows) -> None: self._headers = headers @@ -23,14 +23,14 @@ def __init__(self, headers, rows) -> None: self._table_txt = self._table.get_string() def __repr__(self) -> str: - return self._table_txt + "\n" + self.FOOTER + return self.TITLE + "\n" + self._table_txt def _repr_html_(self) -> str: - return self._table_html + "\n" + self.FOOTER + return self.TITLE + "\n" + self._table_html class ConnectionsTable(Table): - FOOTER = "Active connections" + TITLE = "Active connections:" def __init__(self, headers, rows_maps) -> None: self._rows_maps = rows_maps @@ -48,7 +48,8 @@ def __getitem__(self, key: str): """ This method is provided for backwards compatibility. Before creating ConnectionsTable, `%sql --connections` returned a dictionary, - hence users could retrieve connections using __getitem__ + hence users could retrieve connections using __getitem__. Note that this + was undocumented so we might decide to remove it in the future. """ for row in self._rows_maps: if row["key"] == key: From a6242675002a772ec18a96894d35f15a2c5534b2 Mon Sep 17 00:00:00 2001 From: Eduardo Blancas Date: Sat, 27 May 2023 17:49:45 -0600 Subject: [PATCH 08/21] fix --- src/sql/display.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/sql/display.py b/src/sql/display.py index 53df79e71..c0d4fb2df 100644 --- a/src/sql/display.py +++ b/src/sql/display.py @@ -55,6 +55,10 @@ def __getitem__(self, key: str): if row["key"] == key: return row["connection"] + def __len__(self): + """Also provided for backwards compatibility""" + return len(self._rows_maps) + class Message: """Message for the user""" From 7805999215cc8db3eda12945c13ec27034dfca0e Mon Sep 17 00:00:00 2001 From: Eduardo Blancas Date: Sat, 27 May 2023 17:51:53 -0600 Subject: [PATCH 09/21] update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 007579c60..f4970191b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ * [Feature] Support for printing capture variables using `=<<` syntax (by [@jorisroovers](https://github.com/jorisroovers)) * [Feature] Adds `--persist-replace` argument to replace existing tables when persisting data frames (#440) * [Doc] Hiding connection string when passing `--alias` when opening a connection +* [Doc] Fix `api/magic-sql.md` since it incorrectly stated that listing functions was `--list`, but it's `--connections` +* [Feature] Clearer message display when executing queries, listing connections and persisting data frames ## 0.7.5 (2023-05-24) From 4152dc0d60f03f1e39dd7e352c36867663ff4ecf Mon Sep 17 00:00:00 2001 From: Eduardo Blancas Date: Sat, 27 May 2023 17:55:35 -0600 Subject: [PATCH 10/21] adds missing issue/pr numbers --- CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4970191b..bdecaa43b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,9 +4,9 @@ * [Feature] Support for printing capture variables using `=<<` syntax (by [@jorisroovers](https://github.com/jorisroovers)) * [Feature] Adds `--persist-replace` argument to replace existing tables when persisting data frames (#440) -* [Doc] Hiding connection string when passing `--alias` when opening a connection -* [Doc] Fix `api/magic-sql.md` since it incorrectly stated that listing functions was `--list`, but it's `--connections` -* [Feature] Clearer message display when executing queries, listing connections and persisting data frames +* [Doc] Hiding connection string when passing `--alias` when opening a connection (#432) +* [Doc] Fix `api/magic-sql.md` since it incorrectly stated that listing functions was `--list`, but it's `--connections` (#432) +* [Feature] Clearer message display when executing queries, listing connections and persisting data frames (#432) ## 0.7.5 (2023-05-24) From d2e274cd155063ff5019efdaa4449bf4a50e27b2 Mon Sep 17 00:00:00 2001 From: Eduardo Blancas Date: Sat, 27 May 2023 18:04:16 -0600 Subject: [PATCH 11/21] fix --- src/tests/test_connection.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/tests/test_connection.py b/src/tests/test_connection.py index aead6843e..8a8719de0 100644 --- a/src/tests/test_connection.py +++ b/src/tests/test_connection.py @@ -276,8 +276,20 @@ def test_get_connections(): Connection(engine=create_engine("duckdb://")) assert Connection._get_connections() == [ - {"url": "duckdb://", "current": True, "alias": None}, - {"url": "sqlite://", "current": False, "alias": None}, + { + "url": "duckdb://", + "current": True, + "alias": None, + "key": "duckdb://", + "connection": ANY, + }, + { + "url": "sqlite://", + "current": False, + "alias": None, + "key": "sqlite://", + "connection": ANY, + }, ] From d8826cd6729a28857f724d84b1891b4d9fd75247 Mon Sep 17 00:00:00 2001 From: Eduardo Blancas Date: Sat, 27 May 2023 18:45:37 -0600 Subject: [PATCH 12/21] fix --- src/sql/display.py | 23 ++++++++++++++--------- src/tests/test_run.py | 14 +++++++++++--- 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/src/sql/display.py b/src/sql/display.py index c0d4fb2df..d098983d5 100644 --- a/src/sql/display.py +++ b/src/sql/display.py @@ -33,15 +33,17 @@ class ConnectionsTable(Table): TITLE = "Active connections:" def __init__(self, headers, rows_maps) -> None: - self._rows_maps = rows_maps - def get_values(d): - d = d.copy() - del d["connection"] - del d["key"] + d = {k: v for k, v in d.items() if k not in {"connection", "key"}} return list(d.values()) rows = [get_values(r) for r in rows_maps] + + self._mapping = {} + + for row in rows_maps: + self._mapping[row["key"]] = row["connection"] + super().__init__(headers=headers, rows=rows) def __getitem__(self, key: str): @@ -51,13 +53,16 @@ def __getitem__(self, key: str): hence users could retrieve connections using __getitem__. Note that this was undocumented so we might decide to remove it in the future. """ - for row in self._rows_maps: - if row["key"] == key: - return row["connection"] + return self._mapping[key] + + def __iter__(self): + """Also provided for backwards compatibility""" + for key in self._mapping: + yield key def __len__(self): """Also provided for backwards compatibility""" - return len(self._rows_maps) + return len(self._mapping) class Message: diff --git a/src/tests/test_run.py b/src/tests/test_run.py index 2c8d113a2..c3197dba0 100644 --- a/src/tests/test_run.py +++ b/src/tests/test_run.py @@ -150,9 +150,17 @@ def test_run(monkeypatch, mock_conns, mock_resultset, config_pandas): assert isinstance(output, type(mock_resultset.DataFrame())) -def test_display_affected_rowcount(): - assert display_affected_rowcount(-1) == "Done." - assert display_affected_rowcount(1) == "%d rows affected." % 1 +@pytest.mark.parametrize( + "n, message", + [ + [1, "1 rows affected.\n"], + [0, ""], + ], +) +def test_display_affected_rowcount(capsys, n, message): + display_affected_rowcount(n) + captured = capsys.readouterr() + assert captured.out == message def test_commit_is_called( From 5af9dd28eef65aa13f439b8f3c2028f318d81e76 Mon Sep 17 00:00:00 2001 From: Eduardo Blancas Date: Sat, 27 May 2023 19:27:38 -0600 Subject: [PATCH 13/21] fixes sqlalchemy problem --- src/sql/connection.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/sql/connection.py b/src/sql/connection.py index 2506cfb10..e51f59e1f 100644 --- a/src/sql/connection.py +++ b/src/sql/connection.py @@ -147,15 +147,15 @@ def __init__(self, engine, alias=None): if IS_SQLALCHEMY_ONE: self.metadata = sqlalchemy.MetaData(bind=engine) - url = ( + self.url = ( repr(sqlalchemy.MetaData(bind=engine).bind.url) if IS_SQLALCHEMY_ONE else repr(engine.url) ) - self.session = self._create_session(engine, url) + self.session = self._create_session(engine, self.url) - self.connections[alias or url] = self + self.connections[alias or self.url] = self self.connect_args = None self.alias = alias @@ -378,7 +378,7 @@ def _get_connections(cls): { "current": current, "key": key, - "url": str(engine_url), + "url": conn.url, "alias": conn.alias, "connection": conn, } From df2bde3becfbbf8acd19a6262066d30b572b0097 Mon Sep 17 00:00:00 2001 From: Eduardo Blancas Date: Sat, 27 May 2023 19:44:01 -0600 Subject: [PATCH 14/21] documents connection attributes --- src/sql/connection.py | 34 +++++++++++++++++++++++++++++----- src/tests/test_connection.py | 11 +++++++++++ 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/src/sql/connection.py b/src/sql/connection.py index e51f59e1f..6bd1aea8b 100644 --- a/src/sql/connection.py +++ b/src/sql/connection.py @@ -130,6 +130,30 @@ class Connection: ---------- engine: sqlalchemy.engine.Engine The SQLAlchemy engine to use + + Attributes + ---------- + alias : str or None + The alias passed in the constructor + + engine : sqlalchemy.engine.Engine + The SQLAlchemy engine passed to the constructor + + name : str + A name to identify the connection: {user}@{database_name} + + metadata : Metadata or None + An SQLAlchemy Metadata object (if using SQLAlchemy 2, this is None), + used to retrieve connection information + + url : str + An obfuscated connection string (password hidden) + + dialect : sqlalchemy dialect + A SQLAlchemy dialect object + + session : sqlalchemy session + A SQLAlchemy session object """ # the active connection @@ -139,13 +163,14 @@ class Connection: connections = {} def __init__(self, engine, alias=None): - self.url = engine.url - self.name = self.assign_name(engine) - self.dialect = self.url.get_dialect() + self.alias = alias self.engine = engine + self.name = self.assign_name(engine) if IS_SQLALCHEMY_ONE: self.metadata = sqlalchemy.MetaData(bind=engine) + else: + self.metadata = None self.url = ( repr(sqlalchemy.MetaData(bind=engine).bind.url) @@ -153,12 +178,11 @@ def __init__(self, engine, alias=None): else repr(engine.url) ) + self.dialect = self.engine.url.get_dialect() self.session = self._create_session(engine, self.url) self.connections[alias or self.url] = self - self.connect_args = None - self.alias = alias Connection.current = self @classmethod diff --git a/src/tests/test_connection.py b/src/tests/test_connection.py index 8a8719de0..610edd2aa 100644 --- a/src/tests/test_connection.py +++ b/src/tests/test_connection.py @@ -308,3 +308,14 @@ def test_connections_table(): connections = Connection.connections_table() assert connections._headers == ["current", "url", "alias"] assert connections._rows == [["*", "duckdb://", ""], ["", "sqlite://", ""]] + + +def test_properties(mock_postgres): + conn = Connection.from_connect_str("postgresql://user:topsecret@somedomain.com/db") + + assert "topsecret" not in conn.url + assert "***" in conn.url + assert conn.name == "user@db" + assert isinstance(conn.engine, Engine) + assert conn.dialect + assert conn.session From d02a669b5451256f3ad9622d1c5e2d3719ae5b70 Mon Sep 17 00:00:00 2001 From: Eduardo Blancas Date: Sat, 27 May 2023 19:47:58 -0600 Subject: [PATCH 15/21] lint --- src/sql/connection.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/sql/connection.py b/src/sql/connection.py index 6bd1aea8b..7e0b8b19d 100644 --- a/src/sql/connection.py +++ b/src/sql/connection.py @@ -391,11 +391,6 @@ def _get_connections(cls): for key in sorted(cls.connections): conn = cls.connections[key] - if cls.is_custom_connection(conn): - engine_url = conn.url - else: - engine_url = conn.metadata.bind.url if IS_SQLALCHEMY_ONE else conn.url - current = conn == cls.current connections.append( From 682bee5d08b5ef3c11d648c45a590dd6106f4795 Mon Sep 17 00:00:00 2001 From: Eduardo Blancas Date: Sat, 27 May 2023 19:58:15 -0600 Subject: [PATCH 16/21] update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bdecaa43b..044022ce0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ * [Doc] Hiding connection string when passing `--alias` when opening a connection (#432) * [Doc] Fix `api/magic-sql.md` since it incorrectly stated that listing functions was `--list`, but it's `--connections` (#432) * [Feature] Clearer message display when executing queries, listing connections and persisting data frames (#432) +* [Feature] `%sql --connections` now displays an HTML table in Jupyter and a text-based table in the terminal ## 0.7.5 (2023-05-24) From f186712f70cc3e82fa2d6674d9a3abcec365e1f6 Mon Sep 17 00:00:00 2001 From: Eduardo Blancas Date: Mon, 29 May 2023 10:22:27 -0600 Subject: [PATCH 17/21] adds missing admonition --- doc/community/developer-guide.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/doc/community/developer-guide.md b/doc/community/developer-guide.md index 0aa6672d4..d238197fb 100644 --- a/doc/community/developer-guide.md +++ b/doc/community/developer-guide.md @@ -25,6 +25,10 @@ Before continuing, ensure you have a working [development environment.](https:// ## Displaying messages +```{important} +Use the `sql.display` module instead of `print` for showing feedback to the user. +``` + You can use `message` (contextual information) and `message_success` (successful operations) to show feedback to the user. Here's an example: ```{code-cell} ipython3 @@ -39,6 +43,7 @@ message("Some information") message_success("Some operation finished successfully!") ``` + ## Throwing errors When writing Python libraries, we often throw errors (and display error tracebacks) to let users know that something went wrong. However, JupySQL is an abstraction for executing SQL queries; hence, Python tracebacks a useless to end-users since they expose JupySQL's internals. From 70190a32f24ae18b383c111a4f5131ede688ead3 Mon Sep 17 00:00:00 2001 From: Eduardo Blancas Date: Tue, 30 May 2023 11:28:53 -0600 Subject: [PATCH 18/21] fixes --- src/sql/connection.py | 9 +++++ src/sql/display.py | 2 +- src/tests/conftest.py | 1 + src/tests/test_connection.py | 19 ++++++++++ src/tests/test_magic_display.py | 61 +++++++++++++++++++++++++++++++++ 5 files changed, 91 insertions(+), 1 deletion(-) create mode 100644 src/tests/test_magic_display.py diff --git a/src/sql/connection.py b/src/sql/connection.py index 05d74c2d9..720f44be0 100644 --- a/src/sql/connection.py +++ b/src/sql/connection.py @@ -475,6 +475,15 @@ def close(cls, descriptor): ) conn.session.close() + @classmethod + def close_all(cls): + """Close all active connections""" + connections = Connection.connections.copy() + for key, conn in connections.items(): + conn.close(key) + + cls.connections = {} + def is_custom_connection(conn=None) -> bool: """ Checks if given connection is custom diff --git a/src/sql/display.py b/src/sql/display.py index d098983d5..c7d8dff8b 100644 --- a/src/sql/display.py +++ b/src/sql/display.py @@ -6,7 +6,7 @@ class Table: - """Represents a table""" + """Provides a txt and html representation of tabular data""" TITLE = "" diff --git a/src/tests/conftest.py b/src/tests/conftest.py index 3225b6a65..63ce88830 100644 --- a/src/tests/conftest.py +++ b/src/tests/conftest.py @@ -56,6 +56,7 @@ def ip_empty(): ip_session.register_magics(SqlPlotMagic) ip_session.register_magics(SqlCmdMagic) yield ip_session + Connection.close_all() @pytest.fixture diff --git a/src/tests/test_connection.py b/src/tests/test_connection.py index 845b83a0d..f83c4dfb9 100644 --- a/src/tests/test_connection.py +++ b/src/tests/test_connection.py @@ -3,6 +3,8 @@ import pytest from sqlalchemy import create_engine from sqlalchemy.engine import Engine +from sqlalchemy.exc import ResourceClosedError + import sql.connection from sql.connection import Connection, CustomConnection from IPython.core.error import UsageError @@ -358,3 +360,20 @@ def close(self): def test_custom_connection(conn, expected): is_custom = Connection.is_custom_connection(conn) assert is_custom == expected + + +def test_close_all(ip_empty): + ip_empty.run_cell("%sql duckdb://") + ip_empty.run_cell("%sql sqlite://") + + connections_copy = Connection.connections.copy() + + Connection.close_all() + + with pytest.raises(ResourceClosedError): + connections_copy["sqlite://"].execute("").fetchall() + + with pytest.raises(ResourceClosedError): + connections_copy["duckdb://"].execute("").fetchall() + + assert not Connection.connections diff --git a/src/tests/test_magic_display.py b/src/tests/test_magic_display.py new file mode 100644 index 000000000..cf725a9c6 --- /dev/null +++ b/src/tests/test_magic_display.py @@ -0,0 +1,61 @@ +# a but confusing if it's one or two rows +# open issue: warn users if they're using duckdb and trying to persist a data frame + + +def test_connection_string_displayed(ip_empty, capsys): + ip_empty.run_cell("%sql duckdb://") + ip_empty.run_cell("%sql show tables") + + captured = capsys.readouterr() + assert "Running query in 'duckdb://'" in captured.out + + +def test_custom_connection_display(ip_empty, capsys): + ip_empty.run_cell("import duckdb") + ip_empty.run_cell("custom = duckdb.connect('anotherdb')") + ip_empty.run_cell("%sql custom") + ip_empty.run_cell("%sql show tables") + + captured = capsys.readouterr() + assert "Running query in ' Date: Tue, 30 May 2023 11:33:08 -0600 Subject: [PATCH 19/21] fix --- src/sql/connection.py | 2 +- src/tests/test_magic_display.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sql/connection.py b/src/sql/connection.py index 720f44be0..c6f2e327a 100644 --- a/src/sql/connection.py +++ b/src/sql/connection.py @@ -201,7 +201,7 @@ def __init__(self, engine, alias=None): else repr(engine.url) ) - self.dialect = self.engine.url.get_dialect() + self.dialect = engine.url.get_dialect() self.session = self._create_session(engine, self.url) self.connections[alias or self.url] = self diff --git a/src/tests/test_magic_display.py b/src/tests/test_magic_display.py index cf725a9c6..faec40f5a 100644 --- a/src/tests/test_magic_display.py +++ b/src/tests/test_magic_display.py @@ -10,7 +10,7 @@ def test_connection_string_displayed(ip_empty, capsys): assert "Running query in 'duckdb://'" in captured.out -def test_custom_connection_display(ip_empty, capsys): +def test_custom_connection_display(ip_empty, capsys, tmp_empty): ip_empty.run_cell("import duckdb") ip_empty.run_cell("custom = duckdb.connect('anotherdb')") ip_empty.run_cell("%sql custom") From fedb3549979c4260ccaab36ed478bf0c09c444a8 Mon Sep 17 00:00:00 2001 From: Eduardo Blancas Date: Tue, 30 May 2023 11:49:39 -0600 Subject: [PATCH 20/21] fixes custom connections feedback --- src/sql/display.py | 5 ++++- src/tests/test_display.py | 8 ++++++++ src/tests/test_magic_display.py | 4 ---- 3 files changed, 12 insertions(+), 5 deletions(-) create mode 100644 src/tests/test_display.py diff --git a/src/sql/display.py b/src/sql/display.py index c7d8dff8b..c0c2e7f6c 100644 --- a/src/sql/display.py +++ b/src/sql/display.py @@ -1,6 +1,8 @@ """ A module to display confirmation messages and contextual information to the user """ +import html + from prettytable import PrettyTable from IPython.display import display @@ -70,10 +72,11 @@ class Message: def __init__(self, message, style=None) -> None: self._message = message + self._message_html = html.escape(message) self._style = "" or style def _repr_html_(self): - return f'{self._message}' + return f'{self._message_html}' def __repr__(self) -> str: return self._message diff --git a/src/tests/test_display.py b/src/tests/test_display.py new file mode 100644 index 000000000..08651f6be --- /dev/null +++ b/src/tests/test_display.py @@ -0,0 +1,8 @@ +from sql import display + + +def test_html_escaping(): + message = display.Message("<>") + + assert "<>" in str(message) + assert "<>" in message._repr_html_() diff --git a/src/tests/test_magic_display.py b/src/tests/test_magic_display.py index faec40f5a..c30ed7e75 100644 --- a/src/tests/test_magic_display.py +++ b/src/tests/test_magic_display.py @@ -1,7 +1,3 @@ -# a but confusing if it's one or two rows -# open issue: warn users if they're using duckdb and trying to persist a data frame - - def test_connection_string_displayed(ip_empty, capsys): ip_empty.run_cell("%sql duckdb://") ip_empty.run_cell("%sql show tables") From 3c657eb7a55086952578a818d9a91f436f70fc37 Mon Sep 17 00:00:00 2001 From: Eduardo Blancas Date: Tue, 30 May 2023 12:23:30 -0600 Subject: [PATCH 21/21] deletes outdated test --- src/tests/test_magic.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/tests/test_magic.py b/src/tests/test_magic.py index 2b378880c..c811bbff6 100644 --- a/src/tests/test_magic.py +++ b/src/tests/test_magic.py @@ -462,13 +462,6 @@ def test_connection_args_single_quotes(ip): assert "timeout" in result.result["sqlite:///:memory:"].connect_args -@pytest.mark.skipif(platform.system() == "Windows", reason="failing on windows") -def test_connection_args_double_quotes(ip): - ip.run_cell('%sql --connection_arguments "{\\"timeout\\": 10}" sqlite:///:memory:') - result = ip.run_cell("%sql --connections") - assert "timeout" in result.result["sqlite:///:memory:"].connect_args - - # TODO: support # @with_setup(_setup_author, _teardown_author) # def test_persist_with_connection_info():