-
Notifications
You must be signed in to change notification settings - Fork 125
refactor: break down gbq.py file to several smaller ones #909
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
base: main
Are you sure you want to change the base?
Conversation
pandas_gbq/gbq.py
Outdated
import pandas_gbq.exceptions | ||
from pandas_gbq.exceptions import GenericGBQException, QueryTimeout | ||
from pandas_gbq.contexts import context | ||
from pandas_gbq.exceptions import ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's be explicit about what aliases we are keeping around with __all__
+ a comment explaining why these "unused" aliases are here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__all__
may give us extra burden due to potential star imports. I used # noqa
to mark all unused imports instead.
pandas_gbq/gbq_connector.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're refactoring, I wonder if we actually want GbqConnector at all for our main implementation? It seems like one of those "you don't need a class" situations https://www.youtube.com/watch?v=o9pEzgHorH0 since we aren't actually benefiting from any object oriented isms here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline. We won't address this issue for now.
That said, GbqConnector does seem like an over-engineered OOP class.
The intention of this PR is to re-organize the code to make it easier to implement the dry_run feature: #585
The file
gbq.py
contains too many lines which makes it hard for code navigations.Change summary:
GbqConnector
and its helper functions are relocated fromgbq.py
to a new filegbq_connector.py
gbq.py
are relocated toexceptions.py
file.Context
class andcontext
global instance are relocated fromgbq.py
to a new filecontext.py
.These items are all imported back to the
gbq.py
file to avoid breaking imports of our downstream products.