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

Ab nested #829

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from
Draft

Ab nested #829

wants to merge 17 commits into from

Conversation

tswast
Copy link
Collaborator

@tswast tswast commented Jul 9, 2024

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

Copy link

google-cla bot commented Jul 9, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. api: bigquery Issues related to the googleapis/python-bigquery-dataframes API. labels Jul 9, 2024
Copy link
Collaborator Author

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

I still haven't made it through the full PR, but sharing the comments I have in draft for now.

@@ -62,3 +62,6 @@ system_tests/local_test_setup
# Make sure a generated file isn't accidentally committed.
pylintrc
pylintrc.test
Dockerfile
bigframes/operations/python-bigquery-dataframes.code-workspace
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I imagine we can ignore all *.code-workspace files/directories.

@@ -62,3 +62,6 @@ system_tests/local_test_setup
# Make sure a generated file isn't accidentally committed.
pylintrc
pylintrc.test
Dockerfile
bigframes/operations/python-bigquery-dataframes.code-workspace
pyproject.toml_
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could consider migrating to pyproject.toml in this project. I'm curious what problems it solved for you?


import google.cloud.bigquery
import pandas
import pyarrow as pa
import pyarrow.feather as pa_feather

# schema lineage
from networkx import DiGraph
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nit: Except for the typing module and collections.abc, import modules not classes/functions.

Suggested change
from networkx import DiGraph
import networkx

See: https://google.github.io/styleguide/pyguide.html#22-imports

Aside: Note to self: check that:

  1. networkx and its dependencies have a compatible license
  2. setup.py has networkx as a dependency and
  3. the constraints file(s) have a pin for networkx.

import warnings
from collections.abc import Iterator
from collections import deque
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nit: Except for the typing module and collections.abc, import modules not classes/functions.

Suggested change
from collections import deque
import collections

See: https://google.github.io/styleguide/pyguide.html#22-imports

Aside: when I run "owlbot", it will rearrange these imports alphabetically.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

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-dataframes API. size: xl Pull request size is extra large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants