Skip to content
This repository was archived by the owner on Mar 6, 2026. It is now read-only.

feat: add support for overriding context.engine by magic argument#60

Merged
sycai merged 3 commits intomainfrom
issue16-argument
Sep 24, 2024
Merged

feat: add support for overriding context.engine by magic argument#60
sycai merged 3 commits intomainfrom
issue16-argument

Conversation

@sycai
Copy link
Copy Markdown
Contributor

@sycai sycai commented Sep 23, 2024

Fixes #16 🦕

@sycai sycai requested review from a team, farhan0102 and shobsi September 23, 2024 18:30
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Sep 23, 2024
@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery-magics API. label Sep 23, 2024
@sycai sycai requested a review from tswast September 23, 2024 18:51
Comment on lines +396 to +398
use_bigframes_engine = args.engine or context.engine

if use_bigframes_engine == "bigframes":
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This name feels off to me. Like it would be a boolean but it's not. How about engine_or_default

Suggested change
use_bigframes_engine = args.engine or context.engine
if use_bigframes_engine == "bigframes":
engine_or_default = args.engine or context.engine
if engine_or_default == "bigframes":

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I named it simply"engine"


@pytest.mark.usefixtures("ipython_interactive", "mock_credentials", "bigframes_engine")
@pytest.mark.usefixtures("ipython_interactive", "mock_credentials")
def test_big_query_magic_bigframes_set_in_args():
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: big_query -> bigquery

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right. Updated the name here and for other methods too.

Copy link
Copy Markdown
Contributor

@tswast tswast left a comment

Choose a reason for hiding this comment

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

A few nits. Looks good once addressed.

@sycai sycai merged commit ff57f14 into main Sep 24, 2024
@sycai sycai deleted the issue16-argument branch September 24, 2024 20:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: bigquery Issues related to the googleapis/python-bigquery-magics API. size: m Pull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add an "engine" parameter and context property to support bigframes DataFrame output

3 participants