Reformat based on official Airflow Best Practices #8
Conversation
feluelle
commented
Jun 16, 2022
feluelle
commented
- fix issue with ReplaceVariableGetByJinja
- add new rule ReplaceTopLevelImportByFunctionLevelImport
Codecov Report
@@ Coverage Diff @@
## main #8 +/- ##
==========================================
- Coverage 91.66% 89.15% -2.52%
==========================================
Files 8 6 -2
Lines 156 83 -73
==========================================
- Hits 143 74 -69
+ Misses 13 9 -4
Continue to review full report at Codecov.
|
| @@ -0,0 +1,54 @@ | |||
| # Credits go to GitHub user @isidentical who provided most of the solution. | |||
There was a problem hiding this comment.
@isidentical Is that okay, if I mention you here? Do you want that?
| @@ -0,0 +1,59 @@ | |||
| # Credits go to GitHub user @isidentical who provided most of the solution. | |||
There was a problem hiding this comment.
@isidentical Is that okay, if I mention you here? Do you want that?
| - [x] task decorator instead of PythonOperator and PythonVenvOperator | ||
| - [x] dag decorator instead of DAG | ||
| - [x] jinja string instead of Variable.get | ||
| - [x] Use function-level imports instead of top-level imports[^1] (see [Top level Python Code](https://airflow.apache.org/docs/apache-airflow/stable/best-practices.html#top-level-python-code)) |
There was a problem hiding this comment.
@isidentical In Airflow, not sure if you know the tool, you should avoid top-level code especially imports can cause huge performance issues as Airflow has a parser that continously parses the python file.
| # We remove the old statements. | ||
| for line in split_lines(new_source): | ||
| lines.remove(line) |
There was a problem hiding this comment.
This is what I basically added to your code. Is this how you would do it? Is this safe to do or do I have to take care of edge cases where I can't remove the global imports @isidentical?
There was a problem hiding this comment.
I think using a solution like unimport right after your rules are ran would be better option. There are a lot of edge cases when dealing with import analysis (like try/excepts etc.). https://github.com/hakancelikdev/unimport
There was a problem hiding this comment.
You are probably right - will change that 👍
- fix issue with ReplaceVariableGetByJinja - add new rule ReplaceTopLevelImportByFunctionLevelImport
feluelle
force-pushed
the
feature/top-level-imports
branch
from
6dd8339 to
86972c9
Compare
