Skip to content

[AIRFLOW-6725] Simplify chaining operation in DagFileProcessorManager#7349

Merged
kaxil merged 1 commit intomasterfrom
simplify-chaining-ops
Feb 3, 2020
Merged

[AIRFLOW-6725] Simplify chaining operation in DagFileProcessorManager#7349
kaxil merged 1 commit intomasterfrom
simplify-chaining-ops

Conversation

@kaxil
Copy link
Member

@kaxil kaxil commented Feb 3, 2020

Simplify chaining operation in an IF condition


Issue link: AIRFLOW-6725

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID*
  • Unit tests coverage for changes (not needed for documentation changes)
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

* For document-only changes commit message can start with [AIRFLOW-XXXX].


In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

@kaxil kaxil requested review from mik-laj and potiuk February 3, 2020 14:37
Copy link
Member

@mik-laj mik-laj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I accept it. But what will we do to prevent regression so that similar problems do not arise in the future? Cosmetic changes should be detected automatically because the OOS community cannot force rules otherwise.

@codecov-io
Copy link

Codecov Report

Merging #7349 into master will decrease coverage by 54.26%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #7349       +/-   ##
===========================================
- Coverage   86.35%   32.09%   -54.27%     
===========================================
  Files         871      870        -1     
  Lines       40613    40600       -13     
===========================================
- Hits        35073    13030    -22043     
- Misses       5540    27570    +22030
Impacted Files Coverage Δ
airflow/utils/dag_processing.py 24.71% <0%> (-63.41%) ⬇️
...low/contrib/operators/wasb_delete_blob_operator.py 0% <0%> (-100%) ⬇️
...ing_platform/example_dags/example_display_video.py 0% <0%> (-100%) ⬇️
airflow/contrib/hooks/vertica_hook.py 0% <0%> (-100%) ⬇️
airflow/contrib/sensors/__init__.py 0% <0%> (-100%) ⬇️
airflow/hooks/mssql_hook.py 0% <0%> (-100%) ⬇️
airflow/hooks/webhdfs_hook.py 0% <0%> (-100%) ⬇️
airflow/contrib/sensors/emr_base_sensor.py 0% <0%> (-100%) ⬇️
...irflow/contrib/operators/slack_webhook_operator.py 0% <0%> (-100%) ⬇️
airflow/hooks/jdbc_hook.py 0% <0%> (-100%) ⬇️
... and 741 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb0f6d1...4d2665a. Read the comment docs.

@kaxil kaxil merged commit d02ee3a into master Feb 3, 2020
@kaxil
Copy link
Member Author

kaxil commented Feb 3, 2020

I accept it. But what will we do to prevent regression so that similar problems do not arise in the future? Cosmetic changes should be detected automatically because the OOS community cannot force rules otherwise.

chained-comparison - This pylint rule should, in theory, take care of this.(pylint-dev/pylint#2046)

Need to dig in more detail on why it didn't check this, maybe the file is still not pylint compatible and hence in ignored! list

@kaxil kaxil deleted the simplify-chaining-ops branch February 3, 2020 15:38
galuszkak pushed a commit to FlyrInc/apache-airflow that referenced this pull request Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants