potiuk commented on code in PR #37545:
URL: https://github.com/apache/airflow/pull/37545#discussion_r1496585992
##########
airflow/utils/orm_event_handlers.py:
##########
@@ -23,18 +23,25 @@
import traceback
import sqlalchemy.orm.mapper
-from sqlalchemy import event, exc
+from sqlalchemy import __version__ as sqlalchemy_version, event, exc
from airflow.configuration import conf
log = logging.getLogger(__name__)
+SQL_ALCHEMY_V1 = sqlalchemy_version.startswith("1")
+
def setup_event_handlers(engine):
"""Setups event handlers."""
from airflow.models import import_all_models
- event.listen(sqlalchemy.orm.mapper, "before_configured",
import_all_models, once=True)
+ event.listen(
+ sqlalchemy.orm.mapper if SQL_ALCHEMY_V1 else sqlalchemy.orm.Mapper,
Review Comment:
BTW. Speaking of making decisions and changing them. After looking there I
actually think that it is really not worth to make a common module for that.
Since REALLY you can reduce it to one-liner, better is to use that one-liner
everywhere, rather than extract methods.
It's a rookie mistake to think that DRY is always best. It's not. sometimes
inlining similar code is better for readability. performance and reduced
coupling ( in this case coupling causes too many complications - spmetims we
will want to check for major version, sometimes for >= , sometimes maybe other
check, so it's actually better to inline the checks and leave the freedon on
what condition we want to check. Sometimes we will want to check this:
```python
parse_version(get_version_string('package')) >= Version('2.0.0`)
```
but sometimes
```python
parse_version(get_version_string('package')).major = 1
```
So I think it makes very little sense to extract it to common, code, better
is to make sure that similar patterns are used everywhere and coupling reduced
between those different modules (not mentioning circular dependencies).
--
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]
