Skip to content

Comments

Increasing type coverage for five different providers#11170

Merged
XD-DENG merged 2 commits intoapache:masterfrom
mlgruby:gruby-dev-typing-coverage
Sep 27, 2020
Merged

Increasing type coverage for five different providers#11170
XD-DENG merged 2 commits intoapache:masterfrom
mlgruby:gruby-dev-typing-coverage

Conversation

@mlgruby
Copy link
Contributor

@mlgruby mlgruby commented Sep 27, 2020

Increasing type coverage for multiple providers. Part of: #9708
cc: @kaxil @mik-laj another type coverage one, can I request for review of this PR?


^ 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.

@mlgruby
Copy link
Contributor Author

mlgruby commented Sep 27, 2020

0.000000 dingding
0.000000 opsgenie
0.000000 presto
0.000000 qubole
0.000000 samba
0.000000 zendesk
16.666650 ssh
16.666667 oracle
16.666667 yandex
41.074865 amazon
52.078754 microsoft
69.100860 google
75.000000 postgres
77.619050 databricks ===============> from 23 to 77
77.678575 slack
80.625000 ftp
82.449500 cncf
83.333300 odbc ===================> from 16 to 83
83.333300 openfaas
87.421875 apache
87.500000 redis
88.333340 mysql
88.888900 papermill
90.909100 elasticsearch
96.428575 salesforce
96.875000 mongo
98.076900 imap
100.000000 celery
100.000000 cloudant
100.000000 datadog
100.000000 discord
100.000000 docker
100.000000 exasol ================> from 25 to 100
100.000000 facebook
100.000000 grpc
100.000000 hashicorp
100.000000 http
100.000000 jdbc ==================> from 75 to 100
100.000000 jenkins
100.000000 jira
100.000000 pagerduty
100.000000 plexus
100.000000 segment
100.000000 sendgrid
100.000000 sftp
100.000000 singularity =============> from 25 to 100
100.000000 snowflake
100.000000 sqlite
100.000000 vertica

Copy link
Member

@XD-DENG XD-DENG left a comment

Choose a reason for hiding this comment

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

Too many Any are used where they should be more specific type (say str, case by case).

Doing this (adding Any as type annotation, where it should be a more specific type) causes impression that "we have higher type coverage", but it does not really contribute much real value (in my opinion). def func(x: Any) is no difference from def func(x).

I pointed three such issues below, but there are more.

@mlgruby
Copy link
Contributor Author

mlgruby commented Sep 27, 2020

@XD-DENG Yup, make sense. Added more strict type now!

def __eq__(self, other) -> bool:
def __eq__(self, other: object) -> bool:
if not isinstance(other, RunState):
return NotImplemented
Copy link
Member

Choose a reason for hiding this comment

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

Should it be raise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a special value, which should be returned by binary special methods, we could have a raise for NotImplementedError.

More info here: https://docs.python.org/3/library/constants.html#NotImplemented

@mik-laj mik-laj requested a review from XD-DENG September 27, 2020 16:01
Copy link
Member

@XD-DENG XD-DENG left a comment

Choose a reason for hiding this comment

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

A few more comments. Thanks.

Copy link
Member

@XD-DENG XD-DENG left a comment

Choose a reason for hiding this comment

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

Cool. As long as everything has been taken into consideration.
Thanks @mlgruby .

@XD-DENG XD-DENG merged commit 54353f8 into apache:master Sep 27, 2020
@mlgruby mlgruby deleted the gruby-dev-typing-coverage branch September 30, 2020 23:30
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.

3 participants