john-bodley commented on code in PR #24969:
URL: https://github.com/apache/superset/pull/24969#discussion_r1292021774
##########
superset/daos/dashboard.py:
##########
@@ -181,35 +180,21 @@ def validate_update_slug_uniqueness(dashboard_id: int,
slug: str | None) -> bool
return not db.session.query(dashboard_query.exists()).scalar()
return True
- @staticmethod
Review Comment:
This feels wrong to be part of the `DashboardDAO`, i.e., you're actually
updating the charts and not the dashboard. Either that or added to the
`after_insert`/`after_update` event listener callbacks.
##########
superset/reports/commands/log_prune.py:
##########
@@ -32,9 +32,6 @@ class AsyncPruneReportScheduleLogCommand(BaseCommand):
Prunes logs from all report schedules
"""
- def __init__(self, worker_context: bool = True):
Review Comment:
Never used.
##########
superset/datasets/commands/create.py:
##########
@@ -43,15 +43,11 @@ def __init__(self, data: dict[str, Any]):
def run(self) -> Model:
self.validate()
try:
- # Creates SqlaTable (Dataset)
Review Comment:
Comments seemed redundant and thus unnecessary. Good well named variables
and functions typically remove the need for inline commenting.
##########
superset/databases/ssh_tunnel/commands/create.py:
##########
@@ -39,22 +39,12 @@ def __init__(self, database_id: int, data: dict[str, Any]):
self._properties["database_id"] = database_id
def run(self) -> Model:
+ self.validate()
+
try:
- # Start nested transaction since we are always creating the tunnel
Review Comment:
I don't think this previous logic was necessary given we're only just
calling `SSHTunnelDAO.create(...)`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
