uranusjr commented on a change in pull request #19377:
URL: https://github.com/apache/airflow/pull/19377#discussion_r747352222
##########
File path: airflow/providers/facebook/ads/hooks/ads.py
##########
@@ -118,11 +122,32 @@ def bulk_facebook_report(
:param sleep_time: Time to sleep when async call is happening
:type sleep_time: int
- :return: Facebook Ads API response, converted to Facebook Ads Row
objects
- :rtype: List[AdsInsights]
+ :return: Facebook Ads API response,
+ converted to Facebook Ads Row objects regarding given Account ID
type
+ :rtype: List[AdsInsights] or Dict[str, List[AdsInsights]]
"""
api = self._get_service()
- ad_account = AdAccount(api.get_default_account_id(), api=api)
+ if self.is_backward:
+ return self._facebook_report(
+ account_id=self.facebook_ads_config["account_id"],
+ api=api,
+ params=params,
+ fields=fields,
+ sleep_time=sleep_time,
+ )
Review comment:
This logic does not look… backward to me. Why is the flag called
`is_backward` in the first place? What does backward mean in this context?
##########
File path: airflow/providers/facebook/ads/hooks/ads.py
##########
@@ -98,14 +99,17 @@ def facebook_ads_config(self) -> Dict:
if missing_keys:
message = f"{missing_keys} fields are missing"
raise AirflowException(message)
+ # Solve a way to check in bulk_facebook_report to handle list and dict
return there
+ if type(config["account_id"]) is not list:
Review comment:
```suggestion
if isinstance(config["account_id"], list):
```
This `is_backward` logic is nebulous to me though. Is this “backward if
`account_id` is specified” behaviour specified in Facebook’s API documentation?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
