You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "john-bodley (via GitHub)" <gi...@apache.org> on 2023/06/20 19:42:42 UTC
[GitHub] [superset] john-bodley opened a new pull request, #24465: chore(dao): Add generic type
john-bodley opened a new pull request, #24465:
URL: https://github.com/apache/superset/pull/24465
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
Following up on https://github.com/apache/superset/pull/24331, this PR improves the typing for DAOs by introducing a generic type to ensure that the model being created, updated, deleted, etc. is in accordance with the DAO.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #24465: chore(dao): Add generic type for better type checking
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #24465:
URL: https://github.com/apache/superset/pull/24465#discussion_r1236459818
##########
superset/daos/base.py:
##########
@@ -48,6 +49,11 @@ class BaseDAO:
"""
id_column_name = "id"
+ def __init_subclass__(cls) -> None: # pylint: disable=arguments-differ
+ cls.model_cls = get_args(
+ cls.__orig_bases__[0] # type: ignore # pylint: disable=no-member
+ )[0]
+
Review Comment:
Wow I didn't know Python has added core support for generic types! Adding a reference here for other reviewers: https://peps.python.org/pep-0560/
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #24465: chore(dao): Add generic type for better type checking
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24465:
URL: https://github.com/apache/superset/pull/24465#discussion_r1253409742
##########
superset/annotation_layers/annotations/commands/delete.py:
##########
@@ -38,6 +38,8 @@ def __init__(self, model_id: int):
def run(self) -> Model:
self.validate()
+ assert self._model
Review Comment:
@betodealmeida I’ve been thinking about the DAOs, commands and validation a bit recently and wonder whether we should adopt the “ask for forgiveness” try approach more often which relies less on Python validation and more on the database schema (foreign keys, uniqueness constraints, etc.) for validation, i.e., the DAO or command would fail if invalid.
The benefits are that we would reduce the Python code footprint and prevent possible race conditions.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley merged pull request #24465: chore(dao): Add generic type for better type checking
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley merged PR #24465:
URL: https://github.com/apache/superset/pull/24465
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a diff in pull request #24465: chore(dao): Add generic type for better type checking
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #24465:
URL: https://github.com/apache/superset/pull/24465#discussion_r1253360944
##########
superset/annotation_layers/annotations/commands/delete.py:
##########
@@ -38,6 +38,8 @@ def __init__(self, model_id: int):
def run(self) -> Model:
self.validate()
+ assert self._model
Review Comment:
I have a draft SIP recommending a slight refactor of the commands which address this problem.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #24465: chore(dao): Add generic type for better type checking
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24465:
URL: https://github.com/apache/superset/pull/24465#issuecomment-1599414844
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24465?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#24465](https://app.codecov.io/gh/apache/superset/pull/24465?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (b56f458) into [master](https://app.codecov.io/gh/apache/superset/commit/c3b5d72f2b078eff4ff21213085f0d4ad6d282e9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (c3b5d72) will **decrease** coverage by `10.86%`.
> The diff coverage is `71.31%`.
> :exclamation: Current head b56f458 differs from pull request most recent head 4b1a2f0. Consider uploading reports for the commit 4b1a2f0 to get more accurate results
```diff
@@ Coverage Diff @@
## master #24465 +/- ##
===========================================
- Coverage 68.85% 58.00% -10.86%
===========================================
Files 1901 1901
Lines 73969 73998 +29
Branches 8119 8116 -3
===========================================
- Hits 50931 42922 -8009
- Misses 20927 28965 +8038
Partials 2111 2111
```
| Flag | Coverage Δ | |
|---|---|---|
| postgres | `?` | |
| presto | `53.80% <66.94%> (?)` | |
| python | `60.54% <74.38%> (-22.59%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `54.60% <69.42%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/superset/pull/24465?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ckages/superset-ui-chart-controls/src/constants.ts](https://app.codecov.io/gh/apache/superset/pull/24465?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL2NvbnN0YW50cy50cw==) | `100.00% <ø> (ø)` | |
| [...erset-ui-chart-controls/src/utils/columnChoices.ts](https://app.codecov.io/gh/apache/superset/pull/24465?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3V0aWxzL2NvbHVtbkNob2ljZXMudHM=) | `100.00% <ø> (ø)` | |
| [...packages/superset-ui-core/src/query/types/Query.ts](https://app.codecov.io/gh/apache/superset/pull/24465?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvdHlwZXMvUXVlcnkudHM=) | `100.00% <ø> (ø)` | |
| [superset-frontend/src/SqlLab/actions/sqlLab.js](https://app.codecov.io/gh/apache/superset/pull/24465?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9hY3Rpb25zL3NxbExhYi5qcw==) | `69.16% <ø> (-0.27%)` | :arrow_down: |
| [...d/src/SqlLab/components/SaveDatasetModal/index.tsx](https://app.codecov.io/gh/apache/superset/pull/24465?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NhdmVEYXRhc2V0TW9kYWwvaW5kZXgudHN4) | `50.00% <0.00%> (ø)` | |
| [superset-frontend/src/SqlLab/fixtures.ts](https://app.codecov.io/gh/apache/superset/pull/24465?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9maXh0dXJlcy50cw==) | `100.00% <ø> (ø)` | |
| [...rset-frontend/src/explore/components/SaveModal.tsx](https://app.codecov.io/gh/apache/superset/pull/24465?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9TYXZlTW9kYWwudHN4) | `35.08% <ø> (+0.30%)` | :arrow_up: |
| [...t/annotation\_layers/annotations/commands/delete.py](https://app.codecov.io/gh/apache/superset/pull/24465?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvYW5ub3RhdGlvbl9sYXllcnMvYW5ub3RhdGlvbnMvY29tbWFuZHMvZGVsZXRlLnB5) | `50.00% <0.00%> (-34.00%)` | :arrow_down: |
| [...t/annotation\_layers/annotations/commands/update.py](https://app.codecov.io/gh/apache/superset/pull/24465?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvYW5ub3RhdGlvbl9sYXllcnMvYW5ub3RhdGlvbnMvY29tbWFuZHMvdXBkYXRlLnB5) | `34.04% <0.00%> (-52.92%)` | :arrow_down: |
| [superset/annotation\_layers/commands/delete.py](https://app.codecov.io/gh/apache/superset/pull/24465?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvYW5ub3RhdGlvbl9sYXllcnMvY29tbWFuZHMvZGVsZXRlLnB5) | `46.42% <0.00%> (-42.47%)` | :arrow_down: |
| ... and [44 more](https://app.codecov.io/gh/apache/superset/pull/24465?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
... and [260 files with indirect coverage changes](https://app.codecov.io/gh/apache/superset/pull/24465/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #24465: chore(dao): Add generic type
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24465:
URL: https://github.com/apache/superset/pull/24465#discussion_r1235752061
##########
superset/daos/base.py:
##########
@@ -141,30 +145,23 @@ def create(cls, properties: dict[str, Any], commit: bool = True) -> Model:
return model
@classmethod
- def save(cls, instance_model: Model, commit: bool = True) -> Model:
+ def save(cls, instance_model: T, commit: bool = True) -> None:
"""
Generic for saving models
:raises: DAOCreateFailedError
"""
if cls.model_cls is None:
raise DAOConfigError()
- if not isinstance(instance_model, cls.model_cls):
- raise DAOCreateFailedError(
- "the instance model is not a type of the model class"
- )
try:
db.session.add(instance_model)
if commit:
db.session.commit()
except SQLAlchemyError as ex: # pragma: no cover
db.session.rollback()
raise DAOCreateFailedError(exception=ex) from ex
- return instance_model
Review Comment:
Inconsistent return type detected with the `ChartDAO` implementation. Note there's no need to return `instance_model` as it's passed in as in input.
##########
superset/annotation_layers/annotations/commands/delete.py:
##########
@@ -38,6 +38,8 @@ def __init__(self, model_id: int):
def run(self) -> Model:
self.validate()
+ assert self._model
Review Comment:
This was needed because now Mypy correctly identified that on line #44 `self._model` could have been `None` which didn't adhere to the base class definition.
##########
superset/daos/base.py:
##########
@@ -141,30 +145,23 @@ def create(cls, properties: dict[str, Any], commit: bool = True) -> Model:
return model
@classmethod
- def save(cls, instance_model: Model, commit: bool = True) -> Model:
+ def save(cls, instance_model: T, commit: bool = True) -> None:
"""
Generic for saving models
:raises: DAOCreateFailedError
"""
if cls.model_cls is None:
raise DAOConfigError()
- if not isinstance(instance_model, cls.model_cls):
Review Comment:
This is no longer needed. The Mypy checks ensure this can't happen.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org