dondaum commented on code in PR #56911:
URL: https://github.com/apache/airflow/pull/56911#discussion_r2498284151


##########
providers/discord/src/airflow/providers/discord/hooks/discord_webhook.py:
##########
@@ -19,10 +19,74 @@
 
 import json
 import re
-from typing import Any
+from typing import TYPE_CHECKING, Any
 
-from airflow.exceptions import AirflowException
-from airflow.providers.http.hooks.http import HttpHook
+import aiohttp
+
+from airflow.providers.common.compat.connection import get_async_connection
+from airflow.providers.http.hooks.http import HttpAsyncHook, HttpHook
+
+if TYPE_CHECKING:
+    from airflow.providers.common.compat.sdk import Connection
+
+
+class DiscordCommonHandler:
+    """Contains the common functionality."""
+
+    def get_webhook_endpoint(self, conn: Connection | None, webhook_endpoint: 
str | None) -> str:
+        """
+        Return the default webhook endpoint or override if a webhook_endpoint 
is manually supplied.
+
+        :param conn: Airflow Discord connection
+        :param webhook_endpoint: The manually provided webhook endpoint
+        :return: Webhook endpoint (str) to use
+        """
+        if webhook_endpoint:
+            endpoint = webhook_endpoint
+        elif conn:
+            extra = conn.extra_dejson
+            endpoint = extra.get("webhook_endpoint", "")
+        else:
+            raise ValueError(
+                "Cannot get webhook endpoint: No valid Discord webhook 
endpoint or http_conn_id supplied."
+            )
+
+        # make sure endpoint matches the expected Discord webhook format
+        if not re.fullmatch("webhooks/[0-9]+/[a-zA-Z0-9_-]+", endpoint):
+            raise ValueError(
+                'Expected Discord webhook endpoint in the form of 
"webhooks/{webhook.id}/{webhook.token}".'
+            )
+
+        return endpoint
+
+    def build_discord_payload(
+        self, tts: bool, message: str, username: str | None, avatar_url: str | 
None

Review Comment:
   I changed it to “keyword only.” That's a good approach. Thanks.
   
   I can rename `message` to `content`, but I think we then also need to rename 
the `hooks` init parameters, and most likely those for the operator and 
notfier. Yes, I could make it backwards compatible but I'm not sure it's worth 
the effort. 
   
   It's okay for me to keep it or change it. 
   
   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.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to