You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/10/13 19:16:35 UTC
[GitHub] [airflow] dstandish opened a new pull request, #27043: Allow and prefer non-prefixed extra fields for AsanaHook
dstandish opened a new pull request, #27043:
URL: https://github.com/apache/airflow/pull/27043
From airflow version 2.3, extra prefixes are not required so we enable them here.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #27043: Allow and prefer non-prefixed extra fields for AsanaHook
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #27043:
URL: https://github.com/apache/airflow/pull/27043#discussion_r1001400016
##########
airflow/providers/asana/hooks/asana.py:
##########
@@ -39,8 +68,21 @@ def __init__(self, conn_id: str = default_conn_name, *args, **kwargs) -> None:
super().__init__(*args, **kwargs)
self.connection = self.get_connection(conn_id)
extras = self.connection.extra_dejson
- self.workspace = extras.get("extra__asana__workspace") or None
- self.project = extras.get("extra__asana__project") or None
+ self.workspace = self._get_field(extras, "workspace") or None
+ self.project = self._get_field(extras, "project") or None
+
Review Comment:
yeah i considered that, and it is appealing, but I reasoned that it's not practical because there are frequently subtleties to the way that these params are gotten, idiosyncrasies that require little differences from one provider to another. some of them are done in the same way, but some not. and, it's just backcompat so, do we really want to add a new method to base hook for deprecated backcompat? further, even if we did add this to base hook now, we would not be able to use it until provider min version has this method (which would be a while). alternatively, eventually we could either standardize with appropriate deprecation signaling and make the breaking change for the ones that handle it differently. another option is we could just get rid of the backcompat altogether, but this would force a lot of users to change their connections, and i don't think we should because of the negative experience for users.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #27043: Allow and prefer non-prefixed extra fields for AsanaHook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #27043:
URL: https://github.com/apache/airflow/pull/27043#discussion_r1001987908
##########
airflow/providers/asana/hooks/asana.py:
##########
@@ -39,8 +68,21 @@ def __init__(self, conn_id: str = default_conn_name, *args, **kwargs) -> None:
super().__init__(*args, **kwargs)
self.connection = self.get_connection(conn_id)
extras = self.connection.extra_dejson
- self.workspace = extras.get("extra__asana__workspace") or None
- self.project = extras.get("extra__asana__project") or None
+ self.workspace = self._get_field(extras, "workspace") or None
+ self.project = self._get_field(extras, "project") or None
+
Review Comment:
Nah, you're right. The big one to me is that basehook would mean waiting for airflow core to update so the providers can inherit it. If you do it this way, it's a little more work but it's "immediate" and self-contained. I think you made the right call even if it means some redundant code.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish merged pull request #27043: Allow and prefer non-prefixed extra fields for AsanaHook
Posted by GitBox <gi...@apache.org>.
dstandish merged PR #27043:
URL: https://github.com/apache/airflow/pull/27043
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #27043: Allow and prefer non-prefixed extra fields for AsanaHook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #27043:
URL: https://github.com/apache/airflow/pull/27043#discussion_r1001065846
##########
airflow/providers/asana/hooks/asana.py:
##########
@@ -27,6 +28,34 @@
from airflow.hooks.base import BaseHook
Review Comment:
I want to say that this is the second time I see you using this and it should maybe live in airflow.hooks.base as a shared helper method. Is there a reason not to do it that way?
##########
airflow/providers/asana/hooks/asana.py:
##########
@@ -39,8 +68,21 @@ def __init__(self, conn_id: str = default_conn_name, *args, **kwargs) -> None:
super().__init__(*args, **kwargs)
self.connection = self.get_connection(conn_id)
extras = self.connection.extra_dejson
- self.workspace = extras.get("extra__asana__workspace") or None
- self.project = extras.get("extra__asana__project") or None
+ self.workspace = self._get_field(extras, "workspace") or None
+ self.project = self._get_field(extras, "project") or None
+
Review Comment:
Similarly here, maybe _get_field should live in BaseHook? It would make the future cleanup easier.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org