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 2019/12/29 11:24:05 UTC
[GitHub] [incubator-superset] villebro opened a new pull request #8900:
[WIP] Add and improve type annotations and documentation
villebro opened a new pull request #8900: [WIP] Add and improve type annotations and documentation
URL: https://github.com/apache/incubator-superset/pull/8900
### CATEGORY
Choose one
- [ ] Bug Fix
- [x] Enhancement (new features, refinement)
- [x] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [x] Documentation
### SUMMARY
While working on another fix, I ended up refactoring the existing code, so decided to move these changes into a separate PR.
### TEST PLAN
CI and local testing
### 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
#8900: chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#discussion_r361910109
##########
File path: superset/common/query_context.py
##########
@@ -41,8 +41,8 @@ class QueryContext:
to retrieve the data payload for a given viz.
"""
- cache_type: str = "df"
- enforce_numerical_metrics: bool = True
+ cache_type: ClassVar[str] = "df"
+ enforce_numerical_metrics: ClassVar[bool] = True
Review comment:
@serenajiang I added `ClassVar` to these.
----------------------------------------------------------------
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] serenajiang commented on a change in pull
request #8900: chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
serenajiang commented on a change in pull request #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#discussion_r361876069
##########
File path: superset/connectors/sqla/models.py
##########
@@ -19,7 +19,7 @@
import re
from collections import OrderedDict
from datetime import datetime
-from typing import Any, Dict, List, NamedTuple, Optional, Tuple, Union
+from typing import Any, Dict, Hashable, List, NamedTuple, Optional, Tuple, Union
Review comment:
Might have missed it, but are there any `Hashable`s that aren't `Dict`s?
----------------------------------------------------------------
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 #8900:
chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#issuecomment-569543672
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8900?src=pr&el=h1) Report
> Merging [#8900](https://codecov.io/gh/apache/incubator-superset/pull/8900?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/191aca1fb01b797dae21657021d8fcd1e836c40a?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8900?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8900 +/- ##
=======================================
Coverage 58.97% 58.97%
=======================================
Files 359 359
Lines 11333 11333
Branches 2787 2787
=======================================
Hits 6684 6684
Misses 4471 4471
Partials 178 178
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8900?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/8900?src=pr&el=footer). Last update [191aca1...ff7b33c](https://codecov.io/gh/apache/incubator-superset/pull/8900?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] villebro commented on a change in pull request
#8900: chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#discussion_r361907570
##########
File path: superset/common/query_context.py
##########
@@ -115,17 +106,18 @@ def get_query_result(self, query_object: QueryObject) -> Dict[str, Any]:
"df": df,
}
+ @staticmethod
def df_metrics_to_num( # pylint: disable=invalid-name,no-self-use
- self, df: pd.DataFrame, query_object: QueryObject
+ df: pd.DataFrame, query_object: QueryObject
) -> None:
"""Converting metrics to numeric when pandas.read_sql cannot"""
- metrics = [metric for metric in query_object.metrics]
for col, dtype in df.dtypes.items():
- if dtype.type == np.object_ and col in metrics:
+ if dtype.type == np.object_ and col in query_object.metrics:
df[col] = pd.to_numeric(df[col], errors="coerce")
+ @staticmethod
Review comment:
Thanks for setting me straight on the `ClassVar`! Will update accordingly.
----------------------------------------------------------------
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
#8900: chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#discussion_r361874692
##########
File path: superset/connectors/sqla/models.py
##########
@@ -537,16 +537,9 @@ def select_star(self) -> str:
latest_partition=False,
)
- def get_col(self, col_name: str) -> Optional[Column]:
- columns = self.columns
- for col in columns:
- if col_name == col.column_name:
- return col
- return None
-
Review comment:
This method was functionally an identical copy of `BaseDatasource.get_column` (the parent class), hence was removed and references updated accordingly.
----------------------------------------------------------------
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 #8900:
chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#issuecomment-569543672
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8900?src=pr&el=h1) Report
> Merging [#8900](https://codecov.io/gh/apache/incubator-superset/pull/8900?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/191aca1fb01b797dae21657021d8fcd1e836c40a?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8900?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8900 +/- ##
=======================================
Coverage 58.97% 58.97%
=======================================
Files 359 359
Lines 11333 11333
Branches 2787 2787
=======================================
Hits 6684 6684
Misses 4471 4471
Partials 178 178
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8900?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/8900?src=pr&el=footer). Last update [191aca1...e6fd98a](https://codecov.io/gh/apache/incubator-superset/pull/8900?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] villebro commented on issue #8900: chore:
refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#issuecomment-569544350
@serenajiang it would be great if you could review 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
[GitHub] [incubator-superset] villebro commented on issue #8900: chore:
refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#issuecomment-569598406
Thanks for the comments @serenajiang; this should be ready for another round of review
----------------------------------------------------------------
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 #8900:
chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#issuecomment-569543672
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8900?src=pr&el=h1) Report
> Merging [#8900](https://codecov.io/gh/apache/incubator-superset/pull/8900?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/191aca1fb01b797dae21657021d8fcd1e836c40a?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8900?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8900 +/- ##
=======================================
Coverage 58.97% 58.97%
=======================================
Files 359 359
Lines 11333 11333
Branches 2787 2787
=======================================
Hits 6684 6684
Misses 4471 4471
Partials 178 178
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8900?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/8900?src=pr&el=footer). Last update [191aca1...6ce33da](https://codecov.io/gh/apache/incubator-superset/pull/8900?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] villebro commented on a change in pull request
#8900: chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#discussion_r361874631
##########
File path: superset/connectors/connector_registry.py
##########
@@ -43,11 +43,11 @@ def register_sources(cls, datasource_config: OrderedDict) -> None:
@classmethod
def get_datasource(
cls, datasource_type: str, datasource_id: int, session: Session
- ) -> Optional["BaseDatasource"]:
+ ) -> "BaseDatasource":
return (
session.query(cls.sources[datasource_type])
.filter_by(id=datasource_id)
- .first()
+ .one()
)
Review comment:
`.first()` returns None or the first result, while `.one()` returns only one result, and raises an exception if there are none or more than one row. Returning an `Optional` felt like a simulated exception raise here, hence was changed. See the the code changes below calling this method.
----------------------------------------------------------------
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] serenajiang commented on a change in pull
request #8900: chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
serenajiang commented on a change in pull request #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#discussion_r361875732
##########
File path: superset/connectors/base/models.py
##########
@@ -44,7 +44,7 @@ class BaseDatasource(
baselink: Optional[str] = None # url portion pointing to ModelView endpoint
Review comment:
See comment about `ClassVar` above
----------------------------------------------------------------
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
#8900: chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#discussion_r361907942
##########
File path: superset/connectors/sqla/models.py
##########
@@ -19,7 +19,7 @@
import re
from collections import OrderedDict
from datetime import datetime
-from typing import Any, Dict, List, NamedTuple, Optional, Tuple, Union
+from typing import Any, Dict, Hashable, List, NamedTuple, Optional, Tuple, Union
Review comment:
In this case the `Hashable`s are a list of objects added via `cache_key_wrapper`. Come to think of it, it might be a good idea to check that the objects added via `cache_key_wrapper` are in fact hashable and throw an understandable error if such is not the case. However, will leave that to another PR.
----------------------------------------------------------------
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
#8900: chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#discussion_r361874714
##########
File path: superset/connectors/sqla/models.py
##########
@@ -993,7 +995,6 @@ def mutator(df):
)
else:
df.columns = labels_expected
- return df
Review comment:
No point in returning the mutated object.
----------------------------------------------------------------
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 #8900:
chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#issuecomment-569543672
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8900?src=pr&el=h1) Report
> Merging [#8900](https://codecov.io/gh/apache/incubator-superset/pull/8900?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/191aca1fb01b797dae21657021d8fcd1e836c40a?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8900?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8900 +/- ##
=======================================
Coverage 58.97% 58.97%
=======================================
Files 359 359
Lines 11333 11333
Branches 2787 2787
=======================================
Hits 6684 6684
Misses 4471 4471
Partials 178 178
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8900?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/8900?src=pr&el=footer). Last update [191aca1...2da3b86](https://codecov.io/gh/apache/incubator-superset/pull/8900?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] villebro commented on a change in pull request
#8900: chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#discussion_r361874530
##########
File path: superset/connectors/base/models.py
##########
@@ -415,7 +422,6 @@ def data(self) -> Dict[str, Any]:
"groupby",
"is_dttm",
"type",
- "python_date_format",
Review comment:
`python_date_format` is only needed by `SqlaTable` (child class of `BaseDatasource`), and as `SqlaTable` overrides the `data(self)` method, this isn't really needed here. However, to avoid having to cast `BaseDatasource` to `SqlaTable` later in the code, the `python_date_format` property is defined above by default with a `NotImplementedError()`.
----------------------------------------------------------------
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
#8900: chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#discussion_r361909939
##########
File path: superset/common/query_context.py
##########
@@ -44,29 +45,20 @@ class QueryContext:
cache_type: str = "df"
enforce_numerical_metrics: bool = True
- datasource: BaseDatasource
- queries: List[QueryObject]
- force: bool
- custom_cache_timeout: Optional[int]
Review comment:
Thanks for setting me straight on the ClassVar! Will update accordingly.
----------------------------------------------------------------
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] serenajiang commented on a change in pull
request #8900: chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
serenajiang commented on a change in pull request #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#discussion_r361876457
##########
File path: superset/viz.py
##########
@@ -258,14 +266,14 @@ def process_query_filters(self):
merge_extra_filters(self.form_data)
utils.split_adhoc_filters_into_base_filters(self.form_data)
- def query_obj(self):
+ def query_obj(self) -> Dict[str, Any]:
"""Building a query object"""
form_data = self.form_data
self.process_query_filters()
gb = form_data.get("groupby") or []
metrics = self.all_metrics or []
columns = form_data.get("columns") or []
- groupby = []
+ groupby: List[str] = []
for o in gb + columns:
if o not in groupby:
groupby.append(o)
Review comment:
nit: this bit bothers me. Can we nix this for something like
```python
groupby = list(set(gb + columns))`
```
----------------------------------------------------------------
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
#8900: chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#discussion_r361908108
##########
File path: superset/viz.py
##########
@@ -258,14 +266,14 @@ def process_query_filters(self):
merge_extra_filters(self.form_data)
utils.split_adhoc_filters_into_base_filters(self.form_data)
- def query_obj(self):
+ def query_obj(self) -> Dict[str, Any]:
"""Building a query object"""
form_data = self.form_data
self.process_query_filters()
gb = form_data.get("groupby") or []
metrics = self.all_metrics or []
columns = form_data.get("columns") or []
- groupby = []
+ groupby: List[str] = []
for o in gb + columns:
if o not in groupby:
groupby.append(o)
Review comment:
True, will throw that in.
----------------------------------------------------------------
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] serenajiang commented on a change in pull
request #8900: chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
serenajiang commented on a change in pull request #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#discussion_r361875545
##########
File path: superset/common/query_context.py
##########
@@ -44,29 +45,20 @@ class QueryContext:
cache_type: str = "df"
enforce_numerical_metrics: bool = True
- datasource: BaseDatasource
- queries: List[QueryObject]
- force: bool
- custom_cache_timeout: Optional[int]
Review comment:
Not a typo - class variables are typed as `ClassVar[]` ex.
```python
class A:
class_bool: ClassVar[bool] = True
instance_bool: bool
a = A()
a.instance_bool = True # ok
a.instance_bool = 3 # error
a.class_bool = False # error
```
Looks like typing can be enforced in the constructor as well, but I think it's a little cleaner to put it in the class.
----------------------------------------------------------------
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 #8900:
chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#issuecomment-569543672
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8900?src=pr&el=h1) Report
> Merging [#8900](https://codecov.io/gh/apache/incubator-superset/pull/8900?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/191aca1fb01b797dae21657021d8fcd1e836c40a?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8900?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8900 +/- ##
=======================================
Coverage 58.97% 58.97%
=======================================
Files 359 359
Lines 11333 11333
Branches 2787 2787
=======================================
Hits 6684 6684
Misses 4471 4471
Partials 178 178
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8900?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/8900?src=pr&el=footer). Last update [191aca1...636015b](https://codecov.io/gh/apache/incubator-superset/pull/8900?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] villebro commented on a change in pull request
#8900: chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#discussion_r361907570
##########
File path: superset/common/query_context.py
##########
@@ -115,17 +106,18 @@ def get_query_result(self, query_object: QueryObject) -> Dict[str, Any]:
"df": df,
}
+ @staticmethod
def df_metrics_to_num( # pylint: disable=invalid-name,no-self-use
- self, df: pd.DataFrame, query_object: QueryObject
+ df: pd.DataFrame, query_object: QueryObject
) -> None:
"""Converting metrics to numeric when pandas.read_sql cannot"""
- metrics = [metric for metric in query_object.metrics]
for col, dtype in df.dtypes.items():
- if dtype.type == np.object_ and col in metrics:
+ if dtype.type == np.object_ and col in query_object.metrics:
df[col] = pd.to_numeric(df[col], errors="coerce")
+ @staticmethod
Review comment:
Thanks for setting me straight on the `ClassVar`! Will update accordingly.
----------------------------------------------------------------
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 merged pull request #8900: chore:
refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
villebro merged pull request #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900
----------------------------------------------------------------
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] serenajiang commented on a change in pull
request #8900: chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
serenajiang commented on a change in pull request #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#discussion_r361876457
##########
File path: superset/viz.py
##########
@@ -258,14 +266,14 @@ def process_query_filters(self):
merge_extra_filters(self.form_data)
utils.split_adhoc_filters_into_base_filters(self.form_data)
- def query_obj(self):
+ def query_obj(self) -> Dict[str, Any]:
"""Building a query object"""
form_data = self.form_data
self.process_query_filters()
gb = form_data.get("groupby") or []
metrics = self.all_metrics or []
columns = form_data.get("columns") or []
- groupby = []
+ groupby: List[str] = []
for o in gb + columns:
if o not in groupby:
groupby.append(o)
Review comment:
nit: this bit bothers me. Can we this for something like
```python
groupby = list(set(gb + columns))`
```
----------------------------------------------------------------
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 #8900: [WIP]
Chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #8900: [WIP] Chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#issuecomment-569543672
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8900?src=pr&el=h1) Report
> Merging [#8900](https://codecov.io/gh/apache/incubator-superset/pull/8900?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/191aca1fb01b797dae21657021d8fcd1e836c40a?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8900?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8900 +/- ##
=======================================
Coverage 58.97% 58.97%
=======================================
Files 359 359
Lines 11333 11333
Branches 2787 2787
=======================================
Hits 6684 6684
Misses 4471 4471
Partials 178 178
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8900?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/8900?src=pr&el=footer). Last update [191aca1...0b4b2e1](https://codecov.io/gh/apache/incubator-superset/pull/8900?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] villebro commented on a change in pull request
#8900: chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#discussion_r361874193
##########
File path: superset/common/query_context.py
##########
@@ -44,29 +45,20 @@ class QueryContext:
cache_type: str = "df"
enforce_numerical_metrics: bool = True
- datasource: BaseDatasource
- queries: List[QueryObject]
- force: bool
- custom_cache_timeout: Optional[int]
Review comment:
I think defining types via class variables instead of instance variables was a typo; the types are now enforced in the constructor.
----------------------------------------------------------------
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] serenajiang commented on a change in pull
request #8900: chore: refactor, add typing and fix uncovered errors
Posted by GitBox <gi...@apache.org>.
serenajiang commented on a change in pull request #8900: chore: refactor, add typing and fix uncovered errors
URL: https://github.com/apache/incubator-superset/pull/8900#discussion_r361875554
##########
File path: superset/common/query_context.py
##########
@@ -115,17 +106,18 @@ def get_query_result(self, query_object: QueryObject) -> Dict[str, Any]:
"df": df,
}
+ @staticmethod
def df_metrics_to_num( # pylint: disable=invalid-name,no-self-use
- self, df: pd.DataFrame, query_object: QueryObject
+ df: pd.DataFrame, query_object: QueryObject
) -> None:
"""Converting metrics to numeric when pandas.read_sql cannot"""
- metrics = [metric for metric in query_object.metrics]
for col, dtype in df.dtypes.items():
- if dtype.type == np.object_ and col in metrics:
+ if dtype.type == np.object_ and col in query_object.metrics:
df[col] = pd.to_numeric(df[col], errors="coerce")
+ @staticmethod
Review comment:
noice
----------------------------------------------------------------
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