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

Enable MyPy in strict mode #60

Merged
merged 11 commits into from
Mar 9, 2022
Merged

Enable MyPy in strict mode #60

merged 11 commits into from
Mar 9, 2022

Conversation

kaxil
Copy link
Collaborator

@kaxil kaxil commented Feb 19, 2022

Enable MyPy in a strict mode and run via docker instead of pre-commit so the stubs are installed.

closes #65

@sunank200
Copy link
Collaborator

Added mypy fixes for all amazon operators, sensors along with HTTPSensorAsync.

@@ -108,7 +108,7 @@ def execute(self, context: "Context"):
"Unable to pause cluster since cluster is currently in status: %s", cluster_state
)

def execute_complete(self, context, event=None):
def execute_complete(self, context: Dict[Any, Any], event: Optional[Dict[Any, Any]] = None) -> None:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding Any everywhere doesn't help! It is same as not adding type hints at all

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea of adding type hints is so we know what type a variable/arguments/param is. Adding Any doesn't give any info

Copy link
Collaborator

Choose a reason for hiding this comment

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

In BaseSensorOperator, it uses Any. That was the reason for using Any. link

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any is only used for the return type of execute in that link. Dict[Any, Any] is the same as dict

@kaxil kaxil mentioned this pull request Mar 1, 2022
kaxil added a commit that referenced this pull request Mar 1, 2022
This PR fixes some of the MyPy errors, a subset of #60

Co-authored-by: phanikumv <phani.kumar@astronomer.io>
Co-authored-by: Pankaj <pankaj@astronomer.io>
Co-authored-by: Rajath <92459020+rajaths010494@users.noreply.github.com>
Co-authored-by: Ankit Chaurasia <8670962+sunank200@users.noreply.github.com>
@kaxil kaxil force-pushed the mypy-strict branch 2 times, most recently from 79f6e0a to c26a73f Compare March 1, 2022 03:53
kaxil added a commit that referenced this pull request Mar 1, 2022
This PR fixes some of the MyPy errors, a subset of #60

Co-Authored-By: phanikumv <phani.kumar@astronomer.io>
Co-Authored-By: Pankaj <pankaj@astronomer.io>
Co-Authored-By: Rajath <92459020+rajaths010494@users.noreply.github.com>
Co-Authored-By: Ankit Chaurasia <8670962+sunank200@users.noreply.github.com>
@kaxil kaxil force-pushed the mypy-strict branch 2 times, most recently from 14bf634 to 412fa42 Compare March 2, 2022 00:42
@kaxil kaxil marked this pull request as ready for review March 2, 2022 00:56
@kaxil kaxil marked this pull request as draft March 2, 2022 00:56
@pankajastro
Copy link
Contributor

I can see a couple of inconsistencies on which we need to conclude

def execute(self, context: "Context") -> Any:
def execute(self, context: Dict[Any, Any]) -> None:
def execute(self, context: Context) -> None:
def execute(self, context: Context) -> Any:  # type: ignore[override]
def execute(self, context: Dict[str, Any]) -> None:
def execute_complete(self, context: Dict[Any, Any], event: Optional[Dict[Any, Any]] = None) -> None:
def execute_complete(self, context: Context, event: Any) -> Any:
def execute_complete(self, context: 'Context', event: Any = None) -> None:
async def run(self) -> None:
async def run(self) -> AsyncIterator["TriggerEvent"]:  # type: ignore[override]

@pankajastro pankajastro self-requested a review March 2, 2022 10:06
@kaxil
Copy link
Collaborator Author

kaxil commented Mar 2, 2022

I can see a couple of inconsistencies on which we need to conclude

I agree. For now, let's park it and we can discuss this next week

@codecov
Copy link

codecov bot commented Mar 7, 2022

Codecov Report

Merging #60 (3441429) into main (d20a90b) will decrease coverage by 0.15%.
The diff coverage is 93.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #60      +/-   ##
==========================================
- Coverage   89.92%   89.76%   -0.16%     
==========================================
  Files          35       35              
  Lines        1796     1818      +22     
==========================================
+ Hits         1615     1632      +17     
- Misses        181      186       +5     
Impacted Files Coverage Δ
...ronomer/providers/snowflake/operators/snowflake.py 60.60% <62.50%> (+0.60%) ⬆️
...roviders/cncf/kubernetes/hooks/kubernetes_async.py 59.45% <66.66%> (ø)
astronomer/providers/http/hooks/http.py 88.73% <66.66%> (-1.27%) ⬇️
...mer/providers/amazon/aws/operators/redshift_sql.py 89.28% <75.00%> (-3.03%) ⬇️
...r/providers/amazon/aws/sensors/redshift_cluster.py 84.00% <80.00%> (-2.96%) ⬇️
astronomer/providers/snowflake/hooks/snowflake.py 79.68% <80.00%> (ø)
astronomer/providers/core/sensors/external_task.py 93.54% <85.71%> (-2.75%) ⬇️
...nomer/providers/databricks/operators/databricks.py 80.00% <85.71%> (-1.25%) ⬇️
astronomer/providers/google/cloud/sensors/gcs.py 95.83% <90.90%> (-2.00%) ⬇️
astronomer/providers/amazon/aws/hooks/s3.py 93.93% <100.00%> (ø)
... and 18 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 d20a90b...3441429. Read the comment docs.

kaxil and others added 6 commits March 8, 2022 22:43
Add deps

Update Dockerfile

Update Makefile

Add deps

Update Dockerfile

Update Makefile
Remove redundant casting to boolean

Fix mypy errors in databricks operator and trigger
Add mypy type for bigquery operators and fix test fail for kube
Fix mypy issues for S3KeySensors hooks, operators and triggers

Fix mypy issues for bigquery async

Remove duplicate mypy configs

Fix mypy issues for Amazon async operators and sensors
@kaxil kaxil marked this pull request as ready for review March 9, 2022 02:26
@kaxil kaxil merged commit d244d5a into main Mar 9, 2022
@kaxil kaxil deleted the mypy-strict branch March 9, 2022 02:32
@kaxil
Copy link
Collaborator Author

kaxil commented Mar 9, 2022

🎉 Ok, I have finally got this in !! We also have it running in the CI

OlympuJupiter added a commit to OlympuJupiter/astronomer-providers that referenced this pull request Nov 14, 2022
This PR fixes some of the MyPy errors, a subset of astronomer/astronomer-providers#60

Co-Authored-By: phanikumv <phani.kumar@astronomer.io>
Co-Authored-By: Pankaj <pankaj@astronomer.io>
Co-Authored-By: Rajath <92459020+rajaths010494@users.noreply.github.com>
Co-Authored-By: Ankit Chaurasia <8670962+sunank200@users.noreply.github.com>
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.

Increase Typing Coverage & Enable MyPy
5 participants