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 2022/11/21 12:04:44 UTC
[GitHub] [superset] dpgaspar opened a new pull request, #22178: chore: remove annotation layer FAB CRUD model view
dpgaspar opened a new pull request, #22178:
URL: https://github.com/apache/superset/pull/22178
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### 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 -->
- [ ] 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] dpgaspar merged pull request #22178: chore: remove annotation layer FAB CRUD model view
Posted by GitBox <gi...@apache.org>.
dpgaspar merged PR #22178:
URL: https://github.com/apache/superset/pull/22178
--
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] dpgaspar commented on a diff in pull request #22178: chore: remove annotation layer FAB CRUD model view
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on code in PR #22178:
URL: https://github.com/apache/superset/pull/22178#discussion_r1028304944
##########
superset/views/annotations.py:
##########
@@ -14,117 +14,27 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-from typing import Any, Dict
-
-from flask_appbuilder import CompactCRUDMixin
+from flask_appbuilder import permission_name
from flask_appbuilder.api import expose
-from flask_appbuilder.models.sqla.interface import SQLAInterface
from flask_appbuilder.security.decorators import has_access
-from flask_babel import lazy_gettext as _
-from wtforms.validators import StopValidation
-from superset.constants import MODEL_VIEW_RW_METHOD_PERMISSION_MAP, RouteMethod
-from superset.models.annotations import Annotation, AnnotationLayer
from superset.superset_typing import FlaskResponse
-from superset.views.base import SupersetModelView
-
-
-class StartEndDttmValidator: # pylint: disable=too-few-public-methods
- """
- Validates dttm fields.
- """
- def __call__(self, form: Dict[str, Any], field: Any) -> None:
- if not form["start_dttm"].data and not form["end_dttm"].data:
- raise StopValidation(_("annotation start time or end time is required."))
- if (
- form["end_dttm"].data
- and form["start_dttm"].data
- and form["end_dttm"].data < form["start_dttm"].data
- ):
- raise StopValidation(
- _("Annotation end time must be no earlier than start time.")
- )
Review Comment:
very nice catch! fixing
--
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 #22178: chore: remove annotation layer FAB CRUD model view
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22178:
URL: https://github.com/apache/superset/pull/22178#issuecomment-1322003034
# [Codecov](https://codecov.io/gh/apache/superset/pull/22178?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#22178](https://codecov.io/gh/apache/superset/pull/22178?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ac408ae) into [master](https://codecov.io/gh/apache/superset/commit/cc38400d7120188b4d0b29f03012ab3e06d952b4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cc38400) will **decrease** coverage by `11.47%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #22178 +/- ##
===========================================
- Coverage 67.00% 55.52% -11.48%
===========================================
Files 1835 1834 -1
Lines 69970 69906 -64
Branches 7590 7590
===========================================
- Hits 46880 38818 -8062
- Misses 21124 29122 +7998
Partials 1966 1966
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.44% <ø> (-0.06%)` | :arrow_down: |
| python | `57.41% <ø> (-23.99%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `50.84% <ø> (-0.07%)` | :arrow_down: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/22178?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ontrols/AnnotationLayerControl/AnnotationLayer.jsx](https://codecov.io/gh/apache/superset/pull/22178/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9Bbm5vdGF0aW9uTGF5ZXJDb250cm9sL0Fubm90YXRpb25MYXllci5qc3g=) | `74.85% <ø> (ø)` | |
| [superset/initialization/\_\_init\_\_.py](https://codecov.io/gh/apache/superset/pull/22178/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvaW5pdGlhbGl6YXRpb24vX19pbml0X18ucHk=) | `90.90% <ø> (-0.10%)` | :arrow_down: |
| [superset/views/\_\_init\_\_.py](https://codecov.io/gh/apache/superset/pull/22178/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvX19pbml0X18ucHk=) | `100.00% <ø> (ø)` | |
| [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/22178/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ltcG9ydF9leHBvcnQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset/tags/core.py](https://codecov.io/gh/apache/superset/pull/22178/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdGFncy9jb3JlLnB5) | `4.54% <0.00%> (-95.46%)` | :arrow_down: |
| [superset/key\_value/commands/update.py](https://codecov.io/gh/apache/superset/pull/22178/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `0.00% <0.00%> (-90.91%)` | :arrow_down: |
| [superset/key\_value/commands/delete.py](https://codecov.io/gh/apache/superset/pull/22178/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZS5weQ==) | `0.00% <0.00%> (-87.88%)` | :arrow_down: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/22178/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.19%)` | :arrow_down: |
| [superset/key\_value/commands/delete\_expired.py](https://codecov.io/gh/apache/superset/pull/22178/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZV9leHBpcmVkLnB5) | `0.00% <0.00%> (-84.00%)` | :arrow_down: |
| [superset/dashboards/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/22178/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy9pbXBvcnRlcnMvdjAucHk=) | `15.62% <0.00%> (-76.25%)` | :arrow_down: |
| ... and [285 more](https://codecov.io/gh/apache/superset/pull/22178/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
: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=The+Apache+Software+Foundation)
--
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 #22178: chore: remove annotation layer FAB CRUD model view
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #22178:
URL: https://github.com/apache/superset/pull/22178#discussion_r1028291822
##########
superset/views/annotations.py:
##########
@@ -14,117 +14,27 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-from typing import Any, Dict
-
-from flask_appbuilder import CompactCRUDMixin
+from flask_appbuilder import permission_name
from flask_appbuilder.api import expose
-from flask_appbuilder.models.sqla.interface import SQLAInterface
from flask_appbuilder.security.decorators import has_access
-from flask_babel import lazy_gettext as _
-from wtforms.validators import StopValidation
-from superset.constants import MODEL_VIEW_RW_METHOD_PERMISSION_MAP, RouteMethod
-from superset.models.annotations import Annotation, AnnotationLayer
from superset.superset_typing import FlaskResponse
-from superset.views.base import SupersetModelView
-
-
-class StartEndDttmValidator: # pylint: disable=too-few-public-methods
- """
- Validates dttm fields.
- """
- def __call__(self, form: Dict[str, Any], field: Any) -> None:
- if not form["start_dttm"].data and not form["end_dttm"].data:
- raise StopValidation(_("annotation start time or end time is required."))
- if (
- form["end_dttm"].data
- and form["start_dttm"].data
- and form["end_dttm"].data < form["start_dttm"].data
- ):
- raise StopValidation(
- _("Annotation end time must be no earlier than start time.")
- )
Review Comment:
Note: I verified that the CRUD modal takes care of this validation in the frontend and that this validation is done in the command.
Bycatch while checking the related code: This is trying to append the `Type` to `exceptions`, should be an instance of `AnnotationDatesValidationError`
https://github.com/apache/superset/blob/ac0ff78616119bd5d8bebbb2781e0ef3486c4d19/superset/annotation_layers/annotations/commands/create.py#L73
Why on earth is MyPy not picking this up?
--
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