Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix mypy errors in airflow utils #19914

Merged
merged 1 commit into from
Dec 14, 2021
Merged

Fix mypy errors in airflow utils #19914

merged 1 commit into from
Dec 14, 2021

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Dec 1, 2021

Part of #19891

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
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.

uranusjr
uranusjr reviewed Dec 1, 2021
View reviewed changes
airflow/utils/db.py Outdated Show resolved Hide resolved
airflow/utils/db.py Outdated Show resolved Hide resolved
airflow/utils/dot_renderer.py Show resolved Hide resolved
potiuk
potiuk commented Dec 1, 2021
View reviewed changes
airflow/utils/dot_renderer.py Outdated Show resolved Hide resolved
potiuk
potiuk commented Dec 1, 2021
View reviewed changes
airflow/utils/email.py Outdated Show resolved Hide resolved
potiuk
potiuk commented Dec 1, 2021
View reviewed changes
airflow/utils/session.py Outdated Show resolved Hide resolved
potiuk
potiuk commented Dec 1, 2021
View reviewed changes
airflow/utils/timezone.py Outdated Show resolved Hide resolved
@potiuk potiuk force-pushed the fix-airflow-utils-mypy branch from d5b9c9c to f00879b Compare December 1, 2021 18:48
@potiuk
Copy link
Member Author

potiuk commented Dec 1, 2021

Should be much nicer now :)

@potiuk potiuk force-pushed the fix-airflow-utils-mypy branch 3 times, most recently from 114d287 to 2a13765 Compare December 5, 2021 21:24
ashb
ashb reviewed Dec 7, 2021
View reviewed changes
airflow/utils/log/logging_mixin.py Outdated Show resolved Hide resolved
airflow/utils/timezone.py Show resolved Hide resolved
ashb
ashb reviewed Dec 7, 2021
View reviewed changes
airflow/utils/db.py Outdated Show resolved Hide resolved
@uranusjr uranusjr mentioned this pull request Dec 7, 2021
Merged
@potiuk potiuk force-pushed the fix-airflow-utils-mypy branch 3 times, most recently from 56e0625 to 5b193a8 Compare December 11, 2021 18:54
@potiuk
Copy link
Member Author

potiuk commented Dec 12, 2021

@ashb @uranusjr - should be good to go now.

@potiuk potiuk added the mypy Fixing MyPy problems after bumpin MyPy to 0.990 label Dec 13, 2021
@potiuk potiuk force-pushed the fix-airflow-utils-mypy branch from 5b193a8 to 865d4b8 Compare December 13, 2021 18:15
@potiuk
Copy link
Member Author

potiuk commented Dec 13, 2021

All fixed!

ashb
ashb reviewed Dec 13, 2021
View reviewed changes
airflow/utils/email.py Outdated Show resolved Hide resolved
ashb
ashb reviewed Dec 13, 2021
View reviewed changes
airflow/utils/email.py Outdated Show resolved Hide resolved
ashb
ashb requested changes Dec 13, 2021
View reviewed changes
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

Sorry for the piecemeal reviews -- I have actually looked at the full thing this time, see my two comments

@potiuk
Copy link
Member Author

potiuk commented Dec 13, 2021

No problem. I will take look tomorrow.

@potiuk potiuk mentioned this pull request Dec 13, 2021
Closed
10 tasks
@potiuk potiuk force-pushed the fix-airflow-utils-mypy branch 2 times, most recently from 2c756e5 to 302078f Compare December 13, 2021 23:54
@potiuk potiuk requested a review from ashb December 13, 2021 23:54
@potiuk
Copy link
Member Author

potiuk commented Dec 13, 2021

All fixed :)

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Dec 14, 2021
@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk potiuk force-pushed the fix-airflow-utils-mypy branch from 302078f to 104aa11 Compare December 14, 2021 13:44
@potiuk potiuk force-pushed the fix-airflow-utils-mypy branch from 104aa11 to 8acb0f9 Compare December 14, 2021 15:17
@potiuk
Copy link
Member Author

potiuk commented Dec 14, 2021 •
edited
Loading

@ashb - FYI. I actually changed the "StreamLogWriter" approach - rather than extending from IO Abstract class, I extend from IOBase which has implementations for pretty much all of the abstract methods of IO.

Not only cleaner but it will work for Python3.6 as well (Apparently Python3.6 had some non-implemented abstract methods in IO which caused our tests fail with "Can't instantiate abstract class … with abstract methods”

@potiuk
Copy link
Member Author

potiuk commented Dec 14, 2021

Actually the interesting story was that # TODO: Formally inherit from io.IOBase was there as a comment and I ignored it, thinking that it cannot be that simple :)

@ashb
Copy link
Member

ashb commented Dec 14, 2021

Nice one.

@potiuk potiuk merged commit efd3652 into apache:main Dec 14, 2021
@potiuk potiuk deleted the fix-airflow-utils-mypy branch December 14, 2021 16:52
@jedcunningham jedcunningham added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 7, 2022
@potiuk potiuk restored the fix-airflow-utils-mypy branch April 26, 2022 20:49
@potiuk potiuk deleted the fix-airflow-utils-mypy branch July 29, 2022 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uranusjr uranusjr uranusjr left review comments

@ashb ashb ashb approved these changes

@dstandish dstandish Awaiting requested review from dstandish

@jedcunningham jedcunningham Awaiting requested review from jedcunningham

@kaxil kaxil Awaiting requested review from kaxil

Assignees
No one assigned
Labels
area:logging changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) full tests needed We need to run full set of tests for this PR to merge mypy Fixing MyPy problems after bumpin MyPy to 0.990
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@potiuk @ashb @uranusjr @jedcunningham