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

refactor(api): differentiate between positional and keyword arguments in most expression APIs #10734

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Jan 27, 2025

Closes #9125.

@cpcloud cpcloud added this to the 10.0 milestone Jan 27, 2025
@cpcloud cpcloud added the breaking change Changes that introduce an API break at any level label Jan 27, 2025
@cpcloud cpcloud requested a review from gforsyth January 27, 2025 16:12
@github-actions github-actions bot added tests Issues or PRs related to tests datatypes Issues relating to ibis's datatypes (under `ibis.expr.datatypes`) duckdb The DuckDB backend sql Backends that generate SQL labels Jan 27, 2025
@cpcloud
Copy link
Member Author

cpcloud commented Jan 27, 2025

Once this gets reviewed, I will give the individual commits a BREAKING CHANGE section so that the individual changes/recommendations show up in the 10.0 release notes.

@cpcloud cpcloud force-pushed the positional-keyword-argument-crucible branch 2 times, most recently from a0c329c to 2dbc41c Compare January 28, 2025 10:55
@github-actions github-actions bot added impala The Apache Impala backend pyspark The Apache PySpark backend flink Issues or PRs related to Flink bigquery The BigQuery backend postgres The PostgreSQL backend clickhouse The ClickHouse backend datafusion The Apache DataFusion backend polars The polars backend snowflake The Snowflake backend labels Jan 28, 2025
Copy link
Member

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before I go through the rest of these -- I thought I should check and make sure I'm not missing something -- do we not want to standardize the read_* methods with forced positional and keyword args?

ibis/backends/bigquery/__init__.py Outdated Show resolved Hide resolved
ibis/backends/bigquery/__init__.py Outdated Show resolved Hide resolved
ibis/backends/bigquery/__init__.py Outdated Show resolved Hide resolved
ibis/backends/clickhouse/__init__.py Show resolved Hide resolved
ibis/backends/clickhouse/__init__.py Show resolved Hide resolved
@cpcloud
Copy link
Member Author

cpcloud commented Jan 28, 2025

do we not want to standardize the read_* methods with forced positional and keyword args?

I think ultimately we do, which is why I ended up grinding some of them out earlier this morning.

Copy link
Member

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI failures notwithstanding, this looks good to me. There's some question remaining about whether we always, e.g. say path as the arg name to read_* functions regardless of whether it takes a list of paths, or if we want the distinction. But we don't have to handle that here, since making it positional-only means we can change it without breaking anyone.

ibis/backends/polars/__init__.py Show resolved Hide resolved
ibis/backends/polars/__init__.py Outdated Show resolved Hide resolved
ibis/backends/polars/__init__.py Outdated Show resolved Hide resolved
ibis/backends/polars/__init__.py Outdated Show resolved Hide resolved
ibis/backends/polars/__init__.py Show resolved Hide resolved
ibis/expr/types/arrays.py Outdated Show resolved Hide resolved
ibis/expr/types/arrays.py Outdated Show resolved Hide resolved
ibis/expr/types/arrays.py Outdated Show resolved Hide resolved
ibis/expr/types/json.py Outdated Show resolved Hide resolved
ibis/expr/types/json.py Outdated Show resolved Hide resolved
@cpcloud cpcloud force-pushed the positional-keyword-argument-crucible branch from b53a5a3 to aefebb3 Compare January 29, 2025 12:21
@github-actions github-actions bot added sqlite The SQLite backend mysql The MySQL backend databricks The Databricks backend athena The Amazon Athena backend labels Jan 29, 2025
@cpcloud cpcloud force-pushed the positional-keyword-argument-crucible branch from f89b525 to af56ef3 Compare January 29, 2025 14:25
@github-actions github-actions bot added the docs Documentation related issues or PRs label Jan 29, 2025
@cpcloud cpcloud force-pushed the positional-keyword-argument-crucible branch from af56ef3 to b35c3b0 Compare January 29, 2025 15:31
@github-actions github-actions bot added mssql The Microsoft SQL Server backend trino The Trino backend oracle The Oracle backend exasol Issues related to the exasol backend risingwave The RisingWave backend labels Jan 29, 2025
@cpcloud cpcloud force-pushed the positional-keyword-argument-crucible branch from b35c3b0 to dd400ce Compare January 29, 2025 15:34
@cpcloud cpcloud force-pushed the positional-keyword-argument-crucible branch from dd400ce to dea404a Compare January 29, 2025 15:37
@cpcloud
Copy link
Member Author

cpcloud commented Jan 29, 2025

Ok, well I decided to do the backends as well to the extent possible, might as well avoid 11.0 for as long as possible by doing some grunt work up front.

@cpcloud
Copy link
Member Author

cpcloud commented Jan 29, 2025

Now, the gruntiest part: creating a sequence of commits that will help users understand the breakage 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
athena The Amazon Athena backend bigquery The BigQuery backend breaking change Changes that introduce an API break at any level clickhouse The ClickHouse backend databricks The Databricks backend datafusion The Apache DataFusion backend datatypes Issues relating to ibis's datatypes (under `ibis.expr.datatypes`) docs Documentation related issues or PRs duckdb The DuckDB backend exasol Issues related to the exasol backend flink Issues or PRs related to Flink impala The Apache Impala backend mssql The Microsoft SQL Server backend mysql The MySQL backend oracle The Oracle backend polars The polars backend postgres The PostgreSQL backend pyspark The Apache PySpark backend risingwave The RisingWave backend snowflake The Snowflake backend sql Backends that generate SQL sqlite The SQLite backend tests Issues or PRs related to tests trino The Trino backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor: standardize use of keyword/positional-only arguments
2 participants