-
Notifications
You must be signed in to change notification settings - Fork 77
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.
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
display improvements #432
display improvements #432
Conversation
I think this has to be on the side somewhere eventually, it's fine if we define the logic here and build the widget or CSS view later on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. added minor notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments, the connections table is a nice touch
@yafimvo, @idomic: addressed all the comments. please give it another pass. one important thing: while debugging my tests I realized we had some shared state (a new test would start with existing connections from previous tests), so I added a This led to another problem: I found out a buggy test that started failing because it was inadvertently using a connection from another test. Since the test was checking legacy behavior, I removed the test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm added comments on the tests.
Please wait for Yafim's check on the custom connection before merging.
assert "***" in conn.url | ||
assert conn.name == "user@db" | ||
assert isinstance(conn.engine, Engine) | ||
assert conn.dialect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need those 2?
conn.dialect
, conn.session
assert "<duckdb.DuckDBPyConnection" in txt | ||
assert "sqlite:///my.db" in txt | ||
assert "duckdb:///somedb" in txt | ||
assert "sqlite://" in txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing if sqlite:///my.db
is in txt, sqlite:
will definitely be there. so maybe parameterization is better here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR improves the messages displayed to the user
Describe your changes
--alias
when opening a connection (display improvements #432)api/magic-sql.md
since it incorrectly stated that listing functions was--list
, but it's--connections
(display improvements #432)Connection
attributesIssue number
Closes #X
Checklist before requesting a review
pkgmt format
📚 Documentation preview 📚: https://jupysql--432.org.readthedocs.build/en/432/