Allow and prefer non-prefixed extra fields for AsanaHook #27043
Conversation
From airflow version 2.3, extra prefixes are not required so we enable them here.
| @@ -27,6 +28,34 @@ | |||
| from airflow.hooks.base import BaseHook | |||
|
|
|||
|
|
|||
There was a problem hiding this 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?
| 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 | ||
|
|
There was a problem hiding this comment.
Similarly here, maybe _get_field should live in BaseHook? It would make the future cleanup easier.
There was a problem hiding this 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.
There was a problem hiding this 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.
From airflow version 2.3, extra prefixes are not required so we enable them here.