shahar1 commented on PR #33786: URL: https://github.com/apache/airflow/pull/33786#issuecomment-1694741468
> I am not sure if that's the right set of validations. I do not find loops or if/return particularly troubling. What I was really hoping for was a check that will check for "typical" errors made in operators that are not often caught during review, but they have some negative side effects. > > I am not sure if we want to limit operator's authors from raising warnings/running returns etc. This is far too much IMHO. We do not want to tell the authors "this is how you shoudl do your logic in constructor" - we want to make them aware when they made an obvious mistake. Generally I think we should only run validations that are very clearly violating some not even best but "reasonbly only possible" practice in Airflow. > > I think the rule of thumb should be "You should be able to tell the author EXACTLY how to correct their code and explain why it is wrong". And it should be a mistake that "newbie" contributors are likely to make. > > The issue #29069 was really about this one: > > * when you are using templated arg for something else than just settng it as templated field - Generally speaking the only thing you should do for templated field is you should set it as field. The reason is that this field will not have the right value before "execute" command is fired. > > otherwise having some logic for non-templated fields is perfectly fine. I would not want us to be a "dictactor" to tell people how to write their constructors. > > And there are few other things that are OBVIOUSLY wrong: > > * when you have templated_filed defined for operator but it is not set in the constructor as field, the field is not really templated - it will be missing when templated fields are processed. This is obviously wrong > * when you try to instantiate Hook in the operator's constructor, that's also obviously wrong. You should never do that, because such Hook instantiation will also happen in the DAGFileProcessor when the DAG is parsed. > > I'd start with these three rules, rather than trying to present some constructs that are not obviously wrong. Thanks for the detailed feedback - now I have a better sense of the issue. Drafting this PR for now and I'll try fixing it according to these rules. -- 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]
