uranusjr commented on a change in pull request #19377:
URL: https://github.com/apache/airflow/pull/19377#discussion_r749117689
##########
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:
The implementation still looks wrong to me. The main issue is that
`_get_service` should not _mutate` the hook object.
A better implementation would tie the backward compatibility switch directly
against `facebook_ads_config`, instead of relying on calling `_get_service` to
modify the flag. One way to achieve this is to make the flag a
`cached_property` like `facebook_ads_config`:
```python
@cached_property
def multiple_accounts(self) -> bool:
return isinstance(self.config["account_id"], list)
```
`is_backwards` is also a very bad name because it is unclear what
“backwards” means in the context. It would be better to directly describe what
the difference is instead. The “new” behaviour allows using multiple account,
while the existing one allows only one account, so the property can be called
`multiple_accounts` to signify the difference. (Or it can be called
`single_account` with the semantics flipped.)
--
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]
