potiuk commented on code in PR #37545:
URL: https://github.com/apache/airflow/pull/37545#discussion_r1496573431
##########
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:
create a separate file -propose a name, or find a good one. Generally we
expect to autonomously propose somethign by looking at the code and doing
things similarly. If it looks good, we will accept it, if not, we will suggest
changes.
> BTW, why is airflow.utils.orm_event_handlers a separate file and not part
of airflow.utils.sqlalchemy?
Because it's author chose it to be in a sparate package I guess? There are
no strict rules for that and is (as many naming decisions) subject to
individual person and reviewer. But if you have an idea and need to introduce
some rules for naming that will be easy to answer question `why` then I think
you should propose it.
Gennerally - it's not a science , it's art and that art depends very much
with artist you interact with. You have to accept ambiguities, some level of
uncertainty and show initiative on your own (and accept the fact that commiters
might have different (sometimes between them) opinions, it's also up to you to
figure out how to get to consensus for names etc.
--
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]
