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.

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

Raise NotImplementedError if current_database and list_tables do not apply #2962

Closed
datapythonista opened this issue Sep 10, 2021 · 2 comments · Fixed by #2965
Closed

Raise NotImplementedError if current_database and list_tables do not apply #2962

datapythonista opened this issue Sep 10, 2021 · 2 comments · Fixed by #2965
Labels
ux User experience related issues

Comments

@datapythonista
Copy link
Contributor

datapythonista commented Sep 10, 2021

Following the discussion on #2912, for backends that doesn't support databases, there are 3 options:

Option 1 (original)

>>> con.current_database
None
>>> con.list_databases()
[]

Option 2 (new implemented version, main is an arbitrary name)

>>> con.current_database
'main'
>>> con.list_databases()
['main']

Option 3

>>> con.current_database
Traceback (most recent call last):
...
NotImplementedError: Backend foo does not support multiple databases
>>> con.list_databases()
Traceback (most recent call last):
...
NotImplementedError: Backend foo does not support multiple databases

I think option 1 makes sense in an interactive, but can introduce problems, if for example reusing code from a backend with databases with code reliying on con.current_database in con.list_databases(). Or having to handle special cases like if con.current_database is None:.

Option 2 solves this problem, but relying on a made up database name may not be ideal.

I would settle on option 3, where code won't fail silently. Maybe we can support it with a boolean attribute con.supports_databases, if we think users want to check that, in a nicer way than a try/except.

@cpcloud @jreback how does this sound?

@datapythonista datapythonista added the ux User experience related issues label Sep 10, 2021
@cpcloud
Copy link
Member

cpcloud commented Sep 10, 2021

+1 for option 3.

One issue here is with this phrasing: Backend foo does not support multiple databases.

The problem is this doesn't rule out supporting one database, when in fact I believe that most or all the backends that support databases support multiple databases. I could be wrong there, I haven't looked.

Should we phrase it in terms of "support or not support" instead of "may support one or none, but definitely not many"? :)

@datapythonista
Copy link
Contributor Author

Yes, I think removing the word multiple improves the message, good point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ux User experience related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants