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

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

Merged
merged 3 commits into from
Sep 24, 2024

Conversation

sycai
Copy link
Contributor

@sycai sycai commented Sep 23, 2024

Fixes #16 🦕

@sycai sycai requested review from a team as code owners September 23, 2024 18:30
@sycai sycai requested review from 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
Collaborator

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
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"

@@ -1920,7 +1932,33 @@ def test_big_query_magic_bigframes():
assert bpd.options.bigquery.project == bigquery_magics.context.project


@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
Collaborator

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
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
Collaborator

@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
23 of 24 checks passed
@sycai sycai deleted the issue16-argument branch September 24, 2024 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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