airflow-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [airflow] potiuk commented on a change in pull request #6950: [AIRFLOW-6392] Remove cyclic dependency baseoperator <-> helpers
Date Mon, 30 Dec 2019 11:16:22 GMT
potiuk commented on a change in pull request #6950: [AIRFLOW-6392] Remove cyclic dependency
baseoperator <-> helpers
URL: https://github.com/apache/airflow/pull/6950#discussion_r361961106
 
 

 ##########
 File path: airflow/utils/helpers.py
 ##########
 @@ -142,83 +135,6 @@ def as_flattened_list(iterable):
     return [e for i in iterable for e in i]
 
 
-def chain(*tasks):
 
 Review comment:
   > The existence of the idea of creating an automatic tool does not affect whether we
should or should not make changes.
   
   I don't agree. By having automated tools we might decide that we deliberately not keep
backwards compatibility in some cases more easily  - especially if those changes can easily
and automatically be migrated in the existing DAGs. 
   
   "Backwards compatibility" is simply not as important for 2.0 as it was in 1.10. This decision
has already been made. We have a number of backwards incompatible changes, there is no going
back.
   
   > Others look much less used, although there may be someone who finds these functions.
But in my opinion, this is the choice of the lesser evil.
   I am not sure which one is lesser. If we automate migration for chain method it's not evil
at all for migration. Especially that finally it makes sense for the chain method to be in
baseoperator package (or in some form of operator_helpers but I prefer not to use helpers/managers/etc.
if not neccessary). It makes much more sense for the other helpers method to be there - they
were there originally, simply adding chains and the other method here later was a mistake
that we want to correct now. For me more evil is to have bad naming in future version of airflow.
   
   >> It is not possible to import a single static method into a file. You must always
import the whole class. A similar opinion was expressed in "Google Python Style Guide":
   
   Agree - will make it module function in baseoperator. it makes more sense. It comes from
the old Java background where I prefer to have functions in classes.
   
   

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


With regards,
Apache Git Services

Mime
View raw message