Skip to content

docs: improve docstrings on methods using the database param #11112

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Jun 12, 2025
Merged

docs: improve docstrings on methods using the database param #11112

merged 10 commits into from
Jun 12, 2025
+448 −608

Conversation

NickCrews
Copy link
Contributor

I removed many docstrings from subclasses, which should lead to them inheriting the docstring of the base class. This should make the docstrings easier to maintain and less lilely to drift.

I also linked to the concepts doc that @gforsyth wrote, but that never was included in the index or linked to anywhere. I think it should be surfaced somewhere more obvious.

@NickCrews NickCrews added the docs-preview Add this label to trigger a docs preview label Apr 9, 2025
@github-actions github-actions bot added docs Documentation related issues or PRs impala The Apache Impala backend postgres The PostgreSQL backend sqlite The SQLite backend clickhouse The ClickHouse backend mysql The MySQL backend pyspark The Apache PySpark backend datafusion The Apache DataFusion backend bigquery The BigQuery backend duckdb The DuckDB backend sql Backends that generate SQL snowflake The Snowflake backend mssql The Microsoft SQL Server backend trino The Trino backend druid The Apache Druid backend oracle The Oracle backend exasol Issues related to the exasol backend risingwave The RisingWave backend databricks The Databricks backend athena The Amazon Athena backend and removed docs-preview Add this label to trigger a docs preview labels Apr 9, 2025
gforsyth
gforsyth reviewed Apr 10, 2025
View reviewed changes
Copy link
Member

@gforsyth gforsyth left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for improving the docs, @NickCrews ! One (copy-pasted) spelling error to fix, and I think we should case DuckDB when using it in prose.

I also need to check if ipython style "superhelp" correctly pulls docstrings from the parent class (I'm pretty sure it does, but I remember some issues a while back)

Update: superhelp works just fine.

gforsyth
gforsyth reviewed Apr 10, 2025
View reviewed changes
cpcloud
cpcloud approved these changes Apr 12, 2025
View reviewed changes
Copy link
Member

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

Great work, thanks!

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 12, 2025 •
edited
Loading

The PR title and description conform to the Conventional Commits specification.

@NickCrews NickCrews mentioned this pull request Apr 13, 2025
Closed
@NickCrews NickCrews force-pushed the database-docs branch 2 times, most recently from e364f80 to 6e4909c Compare April 13, 2025 19:34
@github-actions github-actions bot added the flink Issues or PRs related to Flink label Apr 13, 2025
@NickCrews NickCrews force-pushed the database-docs branch 2 times, most recently from d20e674 to 2d80b4f Compare April 13, 2025 19:39
@NickCrews
Copy link
Contributor Author

To get the datafusion tests to pass, I had to dig into getting the current database and catalog for datafusion. I ended up implementing this, and adding HasCurrentDatabase and HasCurrentCatalog abstract base classes so all backends would share the same docstrings. So the scope is a bit larger than before. @cpcloud you probably want to give this another lookover after these adjustments.

@cpcloud
Copy link
Member

cpcloud commented Apr 21, 2025

@NickCrews This seems to break a lot of datafusion functionality for reasons that are unclear to me.

@cpcloud cpcloud marked this pull request as draft April 21, 2025 12:23
@NickCrews NickCrews mentioned this pull request Apr 23, 2025
Merged
@github-actions github-actions bot added the tests Issues or PRs related to tests label Apr 23, 2025
@NickCrews NickCrews marked this pull request as ready for review April 23, 2025 22:25
@NickCrews NickCrews mentioned this pull request May 7, 2025
Merged
@NickCrews
Copy link
Contributor Author

@cpcloud @gforsyth once CI passes this is ready for another review, I fixed the datafusion tests.

cpcloud
cpcloud requested changes Jun 11, 2025
View reviewed changes
NickCrews added 10 commits June 11, 2025 13:36
@NickCrews
docs: improve docstrings on methods using the database param
2cf3225
@NickCrews
fix(datafusion) explicitly convert pandas weakref to str
750bfcc
@NickCrews
fix(druid): stub out NotImplementedErrors if you try to use current_c…
7a6a80a 
…atalog or database param of list_tables()
@NickCrews
chore(impala): don't add CanCreateDatabase as superclass
fcb1ced 
This was breaking tests because it all of a sudden made impala get run for some tests,
but this functionality isn't implemented.
I will do this in a different PR.
@NickCrews
test(datafusion): remove xfail for test of list_catalogs()
097d0a4
@NickCrews
chore: add docstring to HasCurrentCatalog and HasCurrentDatabase
a32da38
@NickCrews
fix(druid) make .list_tables() follow the standard API
0c883c5 
This is breaking.
@NickCrews
chore: remove unused CanCreateDatabase from impala
dde7981
@NickCrews
docs: add backend creation examples to table hierarchy explanation
2c3f593
@NickCrews
chore(docs): add note in list_tables() docstring re duckdb
7b638c2
@cpcloud cpcloud merged commit 458f06d into ibis-project:main Jun 12, 2025
132 of 133 checks passed
@cpcloud
Copy link
Member

cpcloud commented Jun 12, 2025

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gforsyth gforsyth gforsyth left review comments

@cpcloud cpcloud cpcloud approved these changes

Assignees
No one assigned
Labels
athena The Amazon Athena backend bigquery The BigQuery backend clickhouse The ClickHouse backend databricks The Databricks backend datafusion The Apache DataFusion backend docs Documentation related issues or PRs druid The Apache Druid backend 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 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NickCrews @cpcloud @gforsyth