dstandish commented on a change in pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#discussion_r619697979
##########
File path: airflow/providers/google/ads/transfers/ads_to_gcs.py
##########
@@ -92,6 +94,7 @@ def __init__(
page_size: int = 10000,
gzip: bool = False,
impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
+ api_version: str = "v5",
Review comment:
i see, thinking about how to handle case if operator calls hook with
version None
but i would make it so the operator doesn't pass that param to the hook
unless it's not None. then in that case it uses hook default.
then in the hook you can do it in the normal way
```python
def __init__(
self,
api_version: str='v5',
gcp_conn_id: str = "google_cloud_default",
google_ads_conn_id: str = "google_ads_default",
) -> None:
super().__init__()
self.api_version = api_version
...
```
if for some reason it is not possible to do this with the operators
(unlikely) i would do this:
```python
def __init__(
self,
api_version: Optional[str]=None,
gcp_conn_id: str = "google_cloud_default",
google_ads_conn_id: str = "google_ads_default",
) -> None:
super().__init__()
self.api_version = api_version or 'v5'
...
```
true it's a bit hidden, and that's why first suggestion is better, but it's
still better than setting it in many places
if you wanted to make it more brightly visible, you could consider defining
a constant in the module `DEFAULT_API_VERSION` and replace `'v5'` with that
so,
```python
def __init__(
self,
api_version: Optional[str]=None,
gcp_conn_id: str = "google_cloud_default",
google_ads_conn_id: str = "google_ads_default",
) -> None:
super().__init__()
self.api_version = api_version or DEFAULT_API_VERSION
...
```
Lastly, with some clients, not supplying api version means will use latest.
and if that were to be true here, i'd suggest doing that, so we don't need any
version identifier in the code. But i think it may not work that way with gcp.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
