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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org