john-bodley commented on a change in pull request #9824:
URL:
https://github.com/apache/incubator-superset/pull/9824#discussion_r426868698
##########
File path: superset/connectors/druid/models.py
##########
@@ -817,7 +829,7 @@ def granularity(
"year": "P1Y",
}
- granularity: Dict[str, Union[str, float]] = {"type": "period"}
+ granularity = {"type": "period"}
Review comment:
@villebro I guess it differs from the `Granularity` type which I defined
previously (which adds to the confusion). When including the typing here it
causes a conflict with the return type as it can also be a string per line 812.
A couple of learnings from having typed a bunch of Python code:
1. `Optional[...]` values are difficult to handle and thus should be avoided
if possible. It seems they're often used by laziness.
2. Mixed types, i.e., those requiring a `Union[...]` normally indicates poor
quality and unnecessarily complex code.
I think the return type of `granularity` is a clear indication of (2). Also
I believe the true return type should be `Union[Dict[str, Union[str, float]],
str]` but `mypy` complains on this.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
