[GitHub] [airflow] turbaszek commented on a change in pull request #15266: Update Google Ads hook

[GitBox <gi...@apache.org>]

turbaszek commented on a change in pull request #15266:
URL: https://github.com/apache/airflow/pull/15266#discussion_r631589657



##########
File path: airflow/providers/google/ads/operators/ads.py
##########
@@ -89,11 +92,19 @@ def __init__(
         self.google_ads_conn_id = google_ads_conn_id
         self.gzip = gzip
         self.impersonation_chain = impersonation_chain
+        self.api_version = api_version
 
     def execute(self, context: dict) -> str:
         uri = f"gs://{self.bucket}/{self.object_name}"
+        kwargs = {}
+        if self.api_version:
+            kwargs.update(api_version=self.api_version)
 
-        ads_hook = GoogleAdsHook(gcp_conn_id=self.gcp_conn_id, google_ads_conn_id=self.google_ads_conn_id)
+        ads_hook = GoogleAdsHook(
+            gcp_conn_id=self.gcp_conn_id,
+            google_ads_conn_id=self.google_ads_conn_id,
+            **kwargs,
+        )

Review comment:
       Wouldn't it be simpler to do:
   ```
   class GoogleAdsHook:
     default_version = "v5"
     
     def __init__(self, version):
       self.version = version or self.default_version
   ```
   
   In that way we can simply pass `None` values in operators. WDYT?

##########
File path: airflow/providers/google/ads/operators/ads.py
##########
@@ -89,11 +92,19 @@ def __init__(
         self.google_ads_conn_id = google_ads_conn_id
         self.gzip = gzip
         self.impersonation_chain = impersonation_chain
+        self.api_version = api_version
 
     def execute(self, context: dict) -> str:
         uri = f"gs://{self.bucket}/{self.object_name}"
+        kwargs = {}
+        if self.api_version:
+            kwargs.update(api_version=self.api_version)
 
-        ads_hook = GoogleAdsHook(gcp_conn_id=self.gcp_conn_id, google_ads_conn_id=self.google_ads_conn_id)
+        ads_hook = GoogleAdsHook(
+            gcp_conn_id=self.gcp_conn_id,
+            google_ads_conn_id=self.google_ads_conn_id,
+            **kwargs,
+        )

Review comment:
       Wouldn't it be simpler to do:
   ```py
   class GoogleAdsHook:
     default_version = "v5"
     
     def __init__(self, version):
       self.version = version or self.default_version
   ```
   
   In that way we can simply pass `None` values in operators. WDYT?




-- 
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:
users@infra.apache.org