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.

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

Resolve MyPy errors in Cosmos pre-commit #377

Merged
merged 5 commits into from
Jul 24, 2023
Merged

Resolve MyPy errors in Cosmos pre-commit #377

merged 5 commits into from
Jul 24, 2023

Conversation

abhi12mohan
Copy link
Contributor

@abhi12mohan abhi12mohan commented Jul 19, 2023

Description

Resolves all MyPy pre-commit errors. "type: ignore" comments used sparingly and only when needed, i.e. a MyPy solution currently does not exist or deeper code changes need to be made.

Closes: #301

Checklist

  • I have made corresponding changes to the documentation (if required)
  • I have added tests that prove my fix is effective or that my feature works

@netlify
Copy link

netlify bot commented Jul 19, 2023

👷 Deploy Preview for amazing-pothos-a3bca0 processing.

Name Link
🔨 Latest commit 892f784
🔍 Latest deploy log https://app.netlify.com/sites/amazing-pothos-a3bca0/deploys/64bedca167846e00080ffa6e

@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Patch coverage: 92.55% and project coverage change: -0.05% ⚠️

Comparison is base (f153c49) 91.26% compared to head (cfb2123) 91.21%.

❗ Current head cfb2123 differs from pull request most recent head 892f784. Consider uploading reports for the commit 892f784 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #377      +/-   ##
==========================================
- Coverage   91.26%   91.21%   -0.05%     
==========================================
  Files          46       46              
  Lines        1579     1605      +26     
==========================================
+ Hits         1441     1464      +23     
- Misses        138      141       +3     
Files Changed Coverage Δ
cosmos/operators/local.py 80.83% <78.57%> (-0.34%) ⬇️
cosmos/converter.py 98.50% <87.50%> (-1.50%) ⬇️
cosmos/dbt/selector.py 98.61% <88.88%> (-1.39%) ⬇️
cosmos/operators/docker.py 69.76% <92.30%> (ø)
cosmos/operators/kubernetes.py 72.04% <96.55%> (ø)
cosmos/__init__.py 100.00% <100.00%> (ø)
cosmos/airflow/dag.py 100.00% <100.00%> (ø)
cosmos/airflow/graph.py 100.00% <100.00%> (ø)
cosmos/airflow/task_group.py 100.00% <100.00%> (ø)
cosmos/core/airflow.py 93.33% <100.00%> (-0.42%) ⬇️
... and 9 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@abhi12mohan abhi12mohan force-pushed the enable-mypy branch 2 times, most recently from 89d9644 to 8e01a6d Compare July 20, 2023 19:25
@abhi12mohan abhi12mohan changed the title Resolved all MyPy errors in Cosmos pre-commit Resolve all MyPy errors in Cosmos pre-commit Jul 20, 2023
@abhi12mohan abhi12mohan changed the title Resolve all MyPy errors in Cosmos pre-commit Resolve MyPy errors in Cosmos pre-commit Jul 20, 2023
@abhi12mohan abhi12mohan marked this pull request as ready for review July 20, 2023 19:48
@abhi12mohan abhi12mohan requested a review from a team as a code owner July 20, 2023 19:48
@abhi12mohan abhi12mohan requested a review from a team July 20, 2023 19:48
Copy link
Collaborator

@tatiana tatiana left a comment

Choose a reason for hiding this comment

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

Thank you very mich, @abhi12mohan ! It's looking very good. It's a relief to have the type checks in place. I left some minor comments in-line.

cosmos/dataset.py Outdated Show resolved Hide resolved
cosmos/dbt/graph.py Outdated Show resolved Hide resolved
cosmos/dbt/graph.py Outdated Show resolved Hide resolved
cosmos/dbt/parser/project.py Outdated Show resolved Hide resolved
cosmos/dbt/parser/project.py Outdated Show resolved Hide resolved
cosmos/operators/base.py Outdated Show resolved Hide resolved
cosmos/operators/docker.py Show resolved Hide resolved
cosmos/operators/docker.py Show resolved Hide resolved
cosmos/operators/kubernetes.py Show resolved Hide resolved
cosmos/operators/local.py Show resolved Hide resolved
@abhi12mohan abhi12mohan force-pushed the enable-mypy branch 3 times, most recently from d68d3e7 to 7a3c3dc Compare July 21, 2023 19:58
@tatiana
Copy link
Collaborator

tatiana commented Jul 23, 2023

@abhi12mohan, thank you very much for addressing the feedback and creating #382 to follow up on the issues that were ignored in this initial support to MyPy.

Once we rebase and resolve the conflicts, we can merge this PR!

Copy link
Collaborator

@tatiana tatiana left a comment

Choose a reason for hiding this comment

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

Thanks, @abhi12mohan , one last comment - and we can merge! 🎉

dev/dags/dbt/jaffle_shop_python/.user.yml Outdated Show resolved Hide resolved
@tatiana tatiana merged commit 223e4bf into main Jul 24, 2023
37 checks passed
@tatiana tatiana deleted the enable-mypy branch July 24, 2023 20:20
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.

Enable MyPy checks
2 participants