ibis-project / ibis

the portable Python dataframe library
https://ibis-project.org
Apache License 2.0
4.61k stars 563 forks source link

chore: make args required-keywords in make `read_postgres`, `read_sqlite` to be consistent with read_mysql #8712

Closed ncclementi closed 3 months ago

ncclementi commented 3 months ago

Recently, in https://github.com/ibis-project/ibis/pull/8656 we discussed making the arguments of read_mysql required-keywords, and agreed that as a followup we will make this consistent in read_postgresand read_sqlite.

Action Item:

iangow commented 2 months ago

Not clear that this "consistency" is helpful, as it introduces inconsistencies elsewhere. If you search here, you will see many references to schema =. So sometimes for PostgreSQL users, it will be schema = and other times database =. (One can have multiple databases, each with multiple schemas in PostgreSQL.)

gforsyth commented 2 months ago

Hey @iangow -- hopefully on that page any reference to schema= will be exclusively related to an Ibis schema mapping column names to dtypes.

If there are any remaining references to schema as a hierarchical location mapping, those should definitely be removed.

iangow commented 2 months ago

Hey @iangow -- hopefully on that page any reference to schema= will be exclusively related to an Ibis schema mapping column names to dtypes.

If there are any remaining references to schema as a hierarchical location mapping, those should definitely be removed.

It seems there are several places where this is not the case. Sometimes this argument is listed as [deprecated], but not always (e.g., when there's no documentation). For read_postgres() with the DuckDB backend, there was (it seems) no deprecation … the argument just disappeared.

Not sure that every PostgreSQL user (read_postgres()) will know that "database" is being used in the MySQL sense without more documentation.

iangow commented 2 months ago

This note appears at times:

::: {.callout-note} Ibis does not use the word schema to refer to database hierarchy. A collection of tables is referred to as a database. A collection of database is referred to as a catalog. These terms are mapped onto the corresponding features in each backend (where available), regardless of whether the backend itself uses the same terminology. :::

It doesn't render properly in most cases. It looks good under list_databases(), but not under insert() or list_tables() or truncate_table(). Perhaps this note should also go in read_postgres().

gforsyth commented 2 months ago

If an API is marked experimental we may have just removed it outright -- I'll look at the docs rendering issues and also add a note to read_postgres