You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/03/30 14:57:05 UTC
[GitHub] [incubator-superset] dpgaspar opened a new pull request #9418:
[mypy] Enforcing typing for superset.dashboards
dpgaspar opened a new pull request #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418
### CATEGORY
Choose one
- [ ] Bug Fix
- [X] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
Enforcing mypy typing on dashboards module
Following the work done on: https://github.com/apache/incubator-superset/pull/9411
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9418: [mypy] Enforcing typing for superset.dashboards
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418#discussion_r402267027
##########
File path: superset/dashboards/commands/delete.py
##########
@@ -40,7 +41,7 @@ def __init__(self, user: User, model_id: int):
self._model_id = model_id
self._model: Optional[Dashboard] = None
- def run(self):
+ def run(self) -> Optional[Model]:
Review comment:
`BaseDAO.delete` seems to always return `Model` (this should be added to the signature in `superset/dao/base/BaseDAO.delete`), ie. this should also do the same?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9418: [mypy] Enforcing typing for superset.dashboards
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418#discussion_r400328340
##########
File path: superset/dashboards/commands/bulk_delete.py
##########
@@ -40,7 +40,7 @@ def __init__(self, user: User, model_ids: List[int]):
self._model_ids = model_ids
self._models: Optional[List[Dashboard]] = None
- def run(self):
+ def run(self) -> None:
Review comment:
My take is that the abstract is defined has `Optional[Model]` and the implementations can return `None` or `Model`. If we set (for example) `create.run` -> Optional[Model]` then I should check for the model's existence on the return from: https://github.com/apache/incubator-superset/blob/master/superset/dashboards/api.py#L164.
And I'm assuming that the commands succeed and return or raise. What do you think?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #9418: [mypy]
Enforcing typing for superset.dashboards
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418#issuecomment-607288471
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9418?src=pr&el=h1) Report
> Merging [#9418](https://codecov.io/gh/apache/incubator-superset/pull/9418?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/a9ff51bd6133559c71a43318f46c80e0fe913618&el=desc) will **not change** coverage by `%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9418?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9418 +/- ##
=======================================
Coverage 59.01% 59.01%
=======================================
Files 378 378
Lines 12167 12167
Branches 3008 3008
=======================================
Hits 7180 7180
Misses 4806 4806
Partials 181 181
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9418?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9418?src=pr&el=footer). Last update [a9ff51b...9962c2c](https://codecov.io/gh/apache/incubator-superset/pull/9418?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9418: [mypy] Enforcing typing for superset.dashboards
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418#discussion_r400347696
##########
File path: superset/dashboards/commands/bulk_delete.py
##########
@@ -40,7 +40,7 @@ def __init__(self, user: User, model_ids: List[int]):
self._model_ids = model_ids
self._models: Optional[List[Dashboard]] = None
- def run(self):
+ def run(self) -> None:
Review comment:
I prefer the later, i.e., all the `run` signatures having the same return type. It helps to ensures consistency both now and for future classes.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9418: [mypy] Enforcing typing for superset.dashboards
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418#discussion_r400278252
##########
File path: superset/dashboards/commands/bulk_delete.py
##########
@@ -40,7 +40,7 @@ def __init__(self, user: User, model_ids: List[int]):
self._model_ids = model_ids
self._models: Optional[List[Dashboard]] = None
- def run(self):
+ def run(self) -> None:
Review comment:
Should this be `Optional[Model]` per https://github.com/apache/incubator-superset/pull/9416.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9418: [mypy] Enforcing typing for superset.dashboards
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418#discussion_r402267368
##########
File path: superset/dashboards/commands/update.py
##########
@@ -43,9 +44,9 @@ def __init__(self, user: User, model_id: int, data: Dict):
self._actor = user
self._model_id = model_id
self._properties = data.copy()
- self._model: Optional[SqlaTable] = None
+ self._model: Optional[Dashboard] = None
Review comment:
Whoa++
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9418: [mypy] Enforcing typing for superset.dashboards
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418#discussion_r404240583
##########
File path: superset/dashboards/commands/bulk_delete.py
##########
@@ -40,7 +40,7 @@ def __init__(self, user: User, model_ids: List[int]):
self._model_ids = model_ids
self._models: Optional[List[Dashboard]] = None
- def run(self):
+ def run(self) -> None:
Review comment:
@dpgaspar would you mind updating this [line](https://github.com/apache/incubator-superset/blob/master/superset/commands/base.py#L29) as well to ensure consistency?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9418: [mypy] Enforcing typing for superset.dashboards
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418#discussion_r400278252
##########
File path: superset/dashboards/commands/bulk_delete.py
##########
@@ -40,7 +40,7 @@ def __init__(self, user: User, model_ids: List[int]):
self._model_ids = model_ids
self._models: Optional[List[Dashboard]] = None
- def run(self):
+ def run(self) -> None:
Review comment:
Should this be `Optional[Model]` per https://github.com/apache/incubator-superset/pull/9416? Note I presume that the `run` methods should have consistent return type.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9418: [mypy] Enforcing typing for superset.dashboards
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418#discussion_r404452034
##########
File path: superset/commands/base.py
##########
@@ -26,7 +24,7 @@ class BaseCommand(ABC):
"""
@abstractmethod
- def run(self) -> Optional[Model]:
+ def run(self) -> Any:
Review comment:
@john-bodley I'm setting base has Any, a bit wide, another option could be maintaining `Optional[Model]`
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9418: [mypy] Enforcing typing for superset.dashboards
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418#discussion_r404238484
##########
File path: superset/dashboards/commands/bulk_delete.py
##########
@@ -40,7 +40,7 @@ def __init__(self, user: User, model_ids: List[int]):
self._model_ids = model_ids
self._models: Optional[List[Dashboard]] = None
- def run(self):
+ def run(self) -> None:
Review comment:
Ended up reverting typing on command to more explicit type, following @villebro comments and reasoning around:
- We want to migrate logic implemented on models to commands and the return types for `Command.run()` will vary a lot
- `Optional[Model]` will not cover all future use cases and will force us to implement logic that is unnecessary such has testing if the `run` method returned something, when it will always return of raise
- We will not be able to implement a single return type for all `Command.run()` unless creating a very big type definition
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar merged pull request #9418: [mypy]
Enforcing typing for superset.dashboards
Posted by GitBox <gi...@apache.org>.
dpgaspar merged pull request #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9418: [mypy] Enforcing typing for superset.dashboards
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418#discussion_r404238484
##########
File path: superset/dashboards/commands/bulk_delete.py
##########
@@ -40,7 +40,7 @@ def __init__(self, user: User, model_ids: List[int]):
self._model_ids = model_ids
self._models: Optional[List[Dashboard]] = None
- def run(self):
+ def run(self) -> None:
Review comment:
Ended up reverting typing on command to a more explicit type, following @villebro comments and reasoning around:
- We want to migrate logic implemented on models to commands and the return types for `Command.run()` will vary a lot
- `Optional[Model]` will not cover all future use cases and will force us to implement logic that is unnecessary such has testing if the `run` method returned something, when it will always return of raise
- We will not be able to implement a single return type for all `Command.run()` unless creating a very big type definition
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9418: [mypy] Enforcing typing for superset.dashboards
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418#discussion_r400278252
##########
File path: superset/dashboards/commands/bulk_delete.py
##########
@@ -40,7 +40,7 @@ def __init__(self, user: User, model_ids: List[int]):
self._model_ids = model_ids
self._models: Optional[List[Dashboard]] = None
- def run(self):
+ def run(self) -> None:
Review comment:
Should this be `Optional[Model]` per https://github.com/apache/incubator-superset/pull/9416. Note I presume that the `run` methods should have consistent return type.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9418: [mypy]
Enforcing typing for superset.dashboards
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418#issuecomment-609911101
@dpgaspar it seems there's a conflict that needs to be resolved.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #9418:
[mypy] Enforcing typing for superset.dashboards
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418#issuecomment-607288471
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9418?src=pr&el=h1) Report
> Merging [#9418](https://codecov.io/gh/apache/incubator-superset/pull/9418?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/b487834d129fe9c8f969e1b9f44bb5a5a168c15b&el=desc) will **not change** coverage by `%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9418?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9418 +/- ##
=======================================
Coverage 58.77% 58.77%
=======================================
Files 385 385
Lines 12229 12229
Branches 3020 3020
=======================================
Hits 7187 7187
Misses 4858 4858
Partials 184 184
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9418?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9418?src=pr&el=footer). Last update [b487834...ce6b62e](https://codecov.io/gh/apache/incubator-superset/pull/9418?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9418: [mypy] Enforcing typing for superset.dashboards
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418#discussion_r401655907
##########
File path: superset/dashboards/api.py
##########
@@ -161,7 +162,9 @@ def post(self) -> Response:
return self.response_400(message=item.errors)
try:
new_model = CreateDashboardCommand(g.user, item.data).run()
- return self.response(201, id=new_model.id, result=item.data)
+ if new_model:
+ return self.response(201, id=new_model.id, result=item.data)
+ raise DashboardCreateFailedError()
Review comment:
"side effect" for having coherent typing
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9418: [mypy] Enforcing typing for superset.dashboards
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418#discussion_r402258288
##########
File path: superset/charts/commands/delete.py
##########
@@ -39,7 +39,7 @@ class DeleteChartCommand(BaseCommand):
def __init__(self, user: User, model_id: int):
self._actor = user
self._model_id = model_id
- self._model: Optional[SqlaTable] = None
+ self._model: Optional[Slice] = None
Review comment:
Whoa
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9418: [mypy] Enforcing typing for superset.dashboards
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418#discussion_r400333692
##########
File path: superset/dashboards/commands/bulk_delete.py
##########
@@ -40,7 +40,7 @@ def __init__(self, user: User, model_ids: List[int]):
self._model_ids = model_ids
self._models: Optional[List[Dashboard]] = None
- def run(self):
+ def run(self) -> None:
Review comment:
On the other hand we can make them more coherent and always expect `Optional[Model]` this can make future implementation more safely "blind" to the command ifself, just run `run`
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9418: [mypy] Enforcing typing for superset.dashboards
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418#discussion_r402258513
##########
File path: superset/charts/commands/update.py
##########
@@ -46,7 +46,7 @@ def __init__(self, user: User, model_id: int, data: Dict):
self._actor = user
self._model_id = model_id
self._properties = data.copy()
- self._model: Optional[SqlaTable] = None
+ self._model: Optional[Slice] = None
Review comment:
Whoa++
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9418: [mypy] Enforcing typing for superset.dashboards
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9418: [mypy] Enforcing typing for superset.dashboards
URL: https://github.com/apache/incubator-superset/pull/9418#discussion_r402264733
##########
File path: superset/dashboards/commands/create.py
##########
@@ -38,7 +39,7 @@ def __init__(self, user: User, data: Dict):
self._actor = user
self._properties = data.copy()
- def run(self):
+ def run(self) -> Optional[Model]:
Review comment:
Isn't `BaseDAO.create` returning `Model`, ie. this should also do the same? And if so, the changes above should probably also reflect 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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org