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