houqp commented on a change in pull request #8534:
URL: https://github.com/apache/airflow/pull/8534#discussion_r415375821
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -62,16 +63,14 @@ class AwsBaseHook(BaseHook):
def __init__(
self,
- aws_conn_id="aws_default",
+ aws_conn_id: Optional[str] = "aws_default",
Review comment:
the wording in the documentation linked by @zhongjiajie is a little bit
misleading, which probably caused confusion between `Optional` type and
optional argument.
`def foo(a: Optional[str] = 'test'):` tells mypy that argument `a` needs to
be either `None` or a `str`. Since the default value `'test` is a string, this
is a valid use of `Optional` type annotation. `a: Optional[str] = None` is also
a valid annotation.
If we allow `a` to take `None` as value, but annotate it as `def foo(a:
str='test')`, then mypy will not be able to catch errors like `b = a +
'_suffix'` because the annotation `str` tells mypy that `a` will always be a
string. Mypy will also prevent users from calling `foo` with `foo(None)` due to
missing `Optional` in the type annotation, which is not what we want.
Behavior wise, both `aws_conn_id: Optional[str] = None` and `aws_conn_id:
Optional[str] = 'aws_default'` will work for aws hook. I kept it to
`aws_conn_id: Optional[str] = 'aws_default'` because that's what it used to be.
I don't know if it's going to break anything if default is changed to `None`
since there might be users out there actually using `aws_default` connection as
the way to inject global aws credential instead of using default credential
chain lookup.
In AWS land, the best practice is to use default credential chain lookup, so
`aws_conn_id: Optional[str] = None` would be better. It's up to us whether we
are willing risk breaking existing users in order to adopt a better practice. I
don't have a strong opinion on this, but if anyone thinks we should take the
chance for airflow 2.0, I am open to change it to `None`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
