Skip to content

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Dec 1, 2019

Make sure you have checked all steps below.

Jira

Description

  • Here are some details about my PR, including screenshots of any UI changes:

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

@potiuk potiuk requested review from ashb, feluelle and mik-laj December 1, 2019 15:46
@potiuk
Copy link
Member Author

potiuk commented Dec 1, 2019

That's the first of the following PRs after executors unentangling - adds some missing types and cleans up some of the core classes.

@potiuk potiuk changed the title [Airflow 6140] add missing types and pylint [AIRFLOW-6140] Add missing types for some core classes Dec 1, 2019
@potiuk potiuk force-pushed the AIRFLOW-6140-add-missing-types-and-pylint branch 2 times, most recently from eeb2d5c to bf86973 Compare December 1, 2019 18:54
@potiuk potiuk force-pushed the AIRFLOW-6140-add-missing-types-and-pylint branch 2 times, most recently from cd7972a to 70b1b3e Compare December 1, 2019 22:00
@potiuk potiuk changed the title [AIRFLOW-6140] Add missing types for some core classes [AIRFLOW-6140] Add missing types for some core classes. Depends on [AIRFLOW-6004] Dec 1, 2019
@potiuk potiuk force-pushed the AIRFLOW-6140-add-missing-types-and-pylint branch from 70b1b3e to f2c1e66 Compare December 1, 2019 22:32
@codecov-io
Copy link

codecov-io commented Dec 2, 2019

Codecov Report

Merging #6702 into master will decrease coverage by 0.31%.
The diff coverage is 87.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6702      +/-   ##
==========================================
- Coverage    83.9%   83.59%   -0.32%     
==========================================
  Files         668      668              
  Lines       37687    37720      +33     
==========================================
- Hits        31622    31532      -90     
- Misses       6065     6188     +123
Impacted Files Coverage Δ
airflow/configuration.py 89.13% <ø> (-3.63%) ⬇️
airflow/__init__.py 100% <100%> (ø) ⬆️
airflow/serialization/serialized_objects.py 91.22% <100%> (+0.27%) ⬆️
airflow/macros/__init__.py 82.35% <100%> (+1.1%) ⬆️
airflow/config_templates/airflow_local_settings.py 60.86% <65.51%> (-0.84%) ⬇️
airflow/models/baseoperator.py 96.05% <95.74%> (-0.25%) ⬇️
airflow/kubernetes/volume_mount.py 44.44% <0%> (-55.56%) ⬇️
airflow/kubernetes/volume.py 52.94% <0%> (-47.06%) ⬇️
airflow/kubernetes/pod_launcher.py 45.25% <0%> (-46.72%) ⬇️
airflow/kubernetes/refresh_config.py 50.98% <0%> (-23.53%) ⬇️
... and 3 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 e5ff4a1...3669652. Read the comment docs.

@potiuk potiuk force-pushed the AIRFLOW-6140-add-missing-types-and-pylint branch 2 times, most recently from a08663a to 3e30d44 Compare December 2, 2019 21:55
Comment on lines +887 to +893
Copy link
Member

Choose a reason for hiding this comment

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

When is this helpful?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's only when the dag has never been set. We could also throw an exception here (or assert :)), but I think returning empty set is a better choice (because no Dag has no relative ids :D).

With MyPy we now detect many places where None'able (i.e. Optional) field is used to call a method which is great because it really avoids having a mysterious errors during development.

@potiuk potiuk force-pushed the AIRFLOW-6140-add-missing-types-and-pylint branch from 3e30d44 to 8ef99cc Compare December 3, 2019 20:36
@potiuk potiuk changed the title [AIRFLOW-6140] Add missing types for some core classes. Depends on [AIRFLOW-6004] [AIRFLOW-6140] Add missing types for some core classes. Dec 3, 2019
@potiuk potiuk force-pushed the AIRFLOW-6140-add-missing-types-and-pylint branch from 8ef99cc to 7a981bb Compare December 3, 2019 21:19
@potiuk
Copy link
Member Author

potiuk commented Dec 4, 2019

Hey @kaxil - re-reviewed and corrected. Pls take a look .

.travis.yml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

It looks like unrelated changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

True. It was for debugging of a problem. Will revert.

Copy link
Member

@feluelle feluelle left a comment

Choose a reason for hiding this comment

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

LGTM 👍 (after you revert the AIRFLOW_CI_SILENT change)

@potiuk potiuk force-pushed the AIRFLOW-6140-add-missing-types-and-pylint branch from 7a981bb to 3669652 Compare December 4, 2019 14:55
@potiuk potiuk merged commit b355fd6 into apache:master Dec 4, 2019
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.

5 participants