Repository: ploomber/jupysql (python)

Issue:
showing feedback when starting a new connection
we're currently not displaying any feedback when creating a new connection, but we should display something like "Connected to {alias}". This should be configurable and turned off when `feedback=0`

Likewise, the "switching to connection" messages should be turned off when `feedback=0`

you can get the current feedback level with: https://github.com/ploomber/jupysql/blob/master/src/sql/_current.py

Pull request:
show feedback when starting a new connection
## Describe your changes

Show feedback when starting a new connection. This is configurable through setting `%config SqlMagic.feedback = 0` 

## Issue number

Closes #807 

## Checklist before requesting a review

- [x] Performed a self-review of my code
- [x] Formatted my code with [`pkgmt format`](https://ploomber-contributing.readthedocs.io/en/latest/contributing/pr.html#linting-formatting)
- [x] Added [tests](https://ploomber-contributing.readthedocs.io/en/latest/contributing/pr.html#testing) (when necessary).
- [x] Added [docstring](https://ploomber-contributing.readthedocs.io/en/latest/contributing/pr.html#documenting-changes-and-new-features) documentation and update the [changelog](https://ploomber-contributing.readthedocs.io/en/latest/contributing/pr.html#changelog) (when needed)



<!-- readthedocs-preview jupysql start -->
----
:books: Documentation preview :books:: https://jupysql--834.org.readthedocs.build/en/834/

<!-- readthedocs-preview jupysql end -->

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index d2d69c3..23017e7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,7 @@
 
 * [Feature] Improved messages when loading configurations from `pyproject.toml` file.
 * [Feature] Add `--schema/-s` for `%sqlcmd` commands that support `--table/-t` and ensure `--table schema.table` works (#519)
+* [Feature] Show feedback when starting a new connection (#807)
 
 ## 0.10.1 (2023-08-30)
 
diff --git a/src/sql/connection/connection.py b/src/sql/connection/connection.py
index 9dfee40..f92c9f6 100644
--- a/src/sql/connection/connection.py
+++ b/src/sql/connection/connection.py
@@ -265,20 +265,17 @@ class ConnectionManager:
                         _current._config_feedback_normal_or_more()
                         and cls.current != existing
                     ):
-                        display.message(f"Switching to connection {descriptor}")
+                        display.message(f"Switching to connection {descriptor!r}")
                     cls.current = existing
 
                 # passing the same URL but different alias: create a new connection
                 elif existing is None or existing.alias != alias:
-                    if (
-                        _current._config_feedback_normal_or_more()
-                        and cls.current
-                        and cls.current.alias != alias
-                    ):
-                        identifier = alias or descriptor
-                        display.message(
-                            f"Connecting and switching to connection {identifier}"
-                        )
+                    is_connect_and_switch, is_connect = False, False
+                    if cls.current and cls.current.alias != alias:
+                        is_connect_and_switch = True
+                    else:
+                        is_connect = True
+
                     cls.current = cls.from_connect_str(
                         connect_str=descriptor,
                         connect_args=connect_args,
@@ -286,6 +283,14 @@ class ConnectionManager:
                         alias=alias,
                         config=config,
                     )
+                    if _current._config_feedback_normal_or_more():
+                        identifier = alias or cls.current.url
+                        if is_connect_and_switch:
+                            display.message(
+                                f"Connecting and switching to connection {identifier!r}"
+                            )
+                        if is_connect:
+                            display.message(f"Connecting to {identifier!r}")
 
         else:
             if cls.connections:
diff --git a/src/sql/magic_plot.py b/src/sql/magic_plot.py
index 33f2445..1d01bea 100644
--- a/src/sql/magic_plot.py
+++ b/src/sql/magic_plot.py
@@ -1,8 +1,4 @@
-from IPython.core.magic import (
-    Magics,
-    line_magic,
-    magics_class,
-)
+from IPython.core.magic import Magics, line_magic, magics_class
 from IPython.core.magic_arguments import argument, magic_arguments
 from ploomber_core.exceptions import modify_exceptions
 
@@ -14,7 +10,6 @@ except ModuleNotFoundError:
 
 from sql import plot
 from sql.command import SQLPlotCommand
-from sql import exceptions
 from sql import util
 from sql.inspect import is_table_exists
 from sql.store import is_saved_snippet
@@ -28,7 +23,12 @@ class SqlPlotMagic(Magics, Configurable):
 
     @line_magic("sqlplot")
     @magic_arguments()
-    @argument("line", default="", nargs="*", type=str, help="Plot name")
+    @argument(
+        "plot_name",
+        type=str,
+        help="Plot name",
+        choices=["histogram", "hist", "boxplot", "box", "bar", "pie"],
+    )
     @argument("-t", "--table", type=str, help="Table to use", required=True)
     @argument(
         "-c", "--column", type=str, nargs="+", help="Column(s) to use", required=True
@@ -87,20 +87,6 @@ class SqlPlotMagic(Magics, Configurable):
         else:
             column = cmd.args.column
 
-        if not cmd.args.line:
-            plot_str = util.pretty_print(SUPPORTED_PLOTS, last_delimiter="or")
-            raise exceptions.UsageError(
-                "Missing the first argument, must be any of: "
-                f"{plot_str}\n"
-                "Example: %sqlplot histogram"
-            )
-
-        if cmd.args.line[0] not in SUPPORTED_PLOTS + ["hist", "box"]:
-            plot_str = util.pretty_print(SUPPORTED_PLOTS, last_delimiter="or")
-            raise exceptions.UsageError(
-                f"Unknown plot {cmd.args.line[0]!r}. Must be any of: " f"{plot_str}"
-            )
-
         column = util.sanitize_identifier(column)
         table = util.sanitize_identifier(cmd.args.table)
 
@@ -109,7 +95,7 @@ class SqlPlotMagic(Magics, Configurable):
         else:
             with_ = self._check_table_exists(table)
 
-        if cmd.args.line[0] in {"box", "boxplot"}:
+        if cmd.args.plot_name in {"box", "boxplot"}:
             return plot.boxplot(
                 table=table,
                 column=column,
@@ -117,7 +103,7 @@ class SqlPlotMagic(Magics, Configurable):
                 orient=cmd.args.orient,
                 conn=None,
             )
-        elif cmd.args.line[0] in {"hist", "histogram"}:
+        elif cmd.args.plot_name in {"hist", "histogram"}:
             # to avoid passing bins default value when breaks or binwidth is specified
             bin_specified = " --bins " in line or " -b " in line
             breaks_specified = " --breaks " in line or " -B " in line
@@ -135,7 +121,7 @@ class SqlPlotMagic(Magics, Configurable):
                 breaks=cmd.args.breaks,
                 binwidth=cmd.args.binwidth,
             )
-        elif cmd.args.line[0] in {"bar"}:
+        elif cmd.args.plot_name in {"bar"}:
             return plot.bar(
                 table=table,
                 column=column,
@@ -144,7 +130,7 @@ class SqlPlotMagic(Magics, Configurable):
                 show_num=cmd.args.show_numbers,
                 conn=None,
             )
-        elif cmd.args.line[0] in {"pie"}:
+        elif cmd.args.plot_name in {"pie"}:
             return plot.pie(
                 table=table,
                 column=column,


Test patch:
diff --git a/src/tests/test_connection.py b/src/tests/test_connection.py
index 3dccfe1..6fc3d2a 100644
--- a/src/tests/test_connection.py
+++ b/src/tests/test_connection.py
@@ -672,7 +672,7 @@ def test_feedback_when_switching_connection_with_alias(
     ip_empty.run_cell("%sql one")
 
     captured = capsys.readouterr()
-    assert "Switching to connection one" == captured.out.splitlines()[-1]
+    assert "Switching to connection 'one'" == captured.out.splitlines()[-1]
 
 
 def test_feedback_when_switching_connection_with_descriptors(
@@ -684,7 +684,7 @@ def test_feedback_when_switching_connection_with_descriptors(
 
     captured = capsys.readouterr()
     assert (
-        "Connecting and switching to connection sqlite://"
+        "Connecting and switching to connection 'sqlite://'"
         == captured.out.splitlines()[-1]
     )
 
@@ -702,7 +702,7 @@ def test_feedback_when_switching_connection_without_alias(
     ip_empty.run_cell("%sql duckdb://")
 
     captured = capsys.readouterr()
-    assert "Switching to connection duckdb://" == captured.out.splitlines()[-1]
+    assert "Switching to connection 'duckdb://'" == captured.out.splitlines()[-1]
 
 
 def test_feedback_when_switching_connection_with_existing_connection(
@@ -714,18 +714,30 @@ def test_feedback_when_switching_connection_with_existing_connection(
     ip_empty.run_cell("%sql one")
 
     captured = capsys.readouterr()
-    assert "Switching to connection one" == captured.out.splitlines()[-1]
+    assert "Switching to connection 'one'" == captured.out.splitlines()[-1]
 
 
-def test_no_feedback(ip_empty, tmp_empty, capsys):
+@pytest.mark.parametrize(
+    "connection, identifier, feedback",
+    [
+        ("duckdb://", "duckdb://", 1),
+        ("duckdb:// --alias one", "one", 1),
+        ("duckdb://", "duckdb://", 2),
+        ("duckdb:// --alias one", "one", 2),
+    ],
+)
+def test_feedback_when_connecting_to_new_connection(
+    ip_empty, capsys, connection, identifier, feedback
+):
     ip_empty.run_cell("%load_ext sql")
-    ip_empty.run_cell("%sql duckdb://")
+    ip_empty.run_cell(f"%config SqlMagic.feedback = {feedback}")
+    ip_empty.run_cell(f"%sql {connection}")
 
     captured = capsys.readouterr()
-    assert "" == captured.out
+    assert f"Connecting to '{identifier}'" == captured.out.splitlines()[-1]
 
 
-def test_no_switching_connection_feedback_if_disabled(ip_empty, capsys):
+def test_no_connecting_and_switching_connection_feedback_if_disabled(ip_empty, capsys):
     ip_empty.run_cell("%config SqlMagic.feedback = 0")
 
     ip_empty.run_cell("%sql duckdb://")
@@ -737,6 +749,36 @@ def test_no_switching_connection_feedback_if_disabled(ip_empty, capsys):
     assert captured.out == ""
 
 
+@pytest.mark.parametrize(
+    "alias, expected",
+    [(None, "postgresql://user:***@somedomain.com/db"), ("alias", "alias")],
+)
+def test_password_in_feedback_when_connecting_to_new_connection(
+    mock_postgres, ip_empty, capsys, alias, expected
+):
+    url = "postgresql://user:topsecret@somedomain.com/db"
+    _ = ConnectionManager.set(url, displaycon=False, alias=alias)
+    captured = capsys.readouterr()
+    assert f"Connecting to '{expected}'" in captured.out.strip()
+
+
+@pytest.mark.parametrize(
+    "alias, expected",
+    [(None, "postgresql://user:***@somedomain.com/db"), ("alias", "alias")],
+)
+def test_password_in_feedback_when_connecting_and_switching_connection(
+    mock_postgres, ip_empty, capsys, alias, expected
+):
+    ip_empty.run_cell("%sql duckdb://")
+    url = "postgresql://user:topsecret@somedomain.com/db"
+    _ = ConnectionManager.set(url, displaycon=False, alias=alias)
+    captured = capsys.readouterr()
+    assert (
+        f"Connecting and switching to connection '{expected}'"
+        in captured.out.splitlines()[-1]
+    )
+
+
 @pytest.fixture
 def conn_sqlalchemy_duckdb():
     conn = SQLAlchemyConnection(engine=create_engine("duckdb://"))
diff --git a/src/tests/test_magic_plot.py b/src/tests/test_magic_plot.py
index e6f9fec..0cf0a91 100644
--- a/src/tests/test_magic_plot.py
+++ b/src/tests/test_magic_plot.py
@@ -45,19 +45,21 @@ WHERE x > 2
     [
         [
             "%sqlplot someplot -t a -c b",
-            f"Unknown plot 'someplot'. Must be any of: {plot_str}",
+            "argument plot_name: invalid choice: 'someplot' "
+            "(choose from 'histogram', 'hist', 'boxplot', 'box', 'bar', 'pie')",
         ],
         [
             "%sqlplot -t a -c b",
-            f"Missing the first argument, must be any of: {plot_str}",
+            "the following arguments are required: plot_name",
         ],
     ],
+    ids=["invalid_plot_name", "missing_plot_name"],
 )
 def test_validate_plot_name(tmp_empty, ip, cell, error_message):
     with pytest.raises(UsageError) as excinfo:
         ip.run_cell(cell)
 
-    assert excinfo.value.error_type == "UsageError"
+    assert excinfo.typename == "UsageError"
     assert str(error_message).lower() in str(excinfo.value).lower()