korbit-ai[bot] commented on code in PR #31582: URL: https://github.com/apache/superset/pull/31582#discussion_r1907455592
########## superset/migrations/versions/2025-01-08_09-34_d482d51c15ca_remove_legacy_plugins_5_0.py: ########## @@ -0,0 +1,78 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""remove_legacy_plugins_5_0 + +Revision ID: d482d51c15ca +Revises: eb1c288c71c4 +Create Date: 2025-01-08 09:34:57.533332 + +""" + +from alembic import op + +from superset import db +from superset.migrations.shared.migrate_viz.processors import ( + MigrateAreaChart, + MigrateBarChart, + MigrateDistBarChart, + MigrateHeatmapChart, + MigrateHistogramChart, + MigrateLineChart, + MigrateSankey, +) + +# revision identifiers, used by Alembic. +revision = "d482d51c15ca" +down_revision = "eb1c288c71c4" + + +def upgrade(): + bind = op.get_bind() + session = db.Session(bind=bind) + try: + MigrateAreaChart.upgrade(session) + MigrateBarChart.upgrade(session) + MigrateDistBarChart.upgrade(session) + MigrateHeatmapChart.upgrade(session) + MigrateHistogramChart.upgrade(session) + MigrateLineChart.upgrade(session) + MigrateSankey.upgrade(session) + session.commit() + except Exception as e: + session.rollback() + raise Exception(f"Error upgrading legacy viz types: {e}") from e Review Comment: ### Incorrect Error Message in Downgrade Function <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The error message in the downgrade function incorrectly states 'Error upgrading legacy viz types' when it should be 'Error downgrading legacy viz types' ###### Why this matters This incorrect error message could mislead developers debugging issues during a downgrade operation, potentially causing confusion and increasing time to resolve issues. ###### Suggested change Change the error message to: raise Exception(f"Error downgrading legacy viz types: {e}") from e </details> ###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews. <!--- korbi internal id:5b41490c-6c54-41b4-95a3-a41cfe6d458c --> ########## superset/migrations/versions/2025-01-08_09-34_d482d51c15ca_remove_legacy_plugins_5_0.py: ########## @@ -0,0 +1,78 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""remove_legacy_plugins_5_0 + +Revision ID: d482d51c15ca +Revises: eb1c288c71c4 +Create Date: 2025-01-08 09:34:57.533332 + +""" + +from alembic import op + +from superset import db +from superset.migrations.shared.migrate_viz.processors import ( + MigrateAreaChart, + MigrateBarChart, + MigrateDistBarChart, + MigrateHeatmapChart, + MigrateHistogramChart, + MigrateLineChart, + MigrateSankey, +) + +# revision identifiers, used by Alembic. +revision = "d482d51c15ca" +down_revision = "eb1c288c71c4" + + +def upgrade(): + bind = op.get_bind() + session = db.Session(bind=bind) + try: + MigrateAreaChart.upgrade(session) + MigrateBarChart.upgrade(session) + MigrateDistBarChart.upgrade(session) + MigrateHeatmapChart.upgrade(session) + MigrateHistogramChart.upgrade(session) + MigrateLineChart.upgrade(session) + MigrateSankey.upgrade(session) + session.commit() + except Exception as e: + session.rollback() + raise Exception(f"Error upgrading legacy viz types: {e}") from e Review Comment: ### Non-specific error message in chart migration <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The error message is too generic and doesn't indicate which specific chart migration failed. ###### Why this matters When an error occurs, it will be difficult to identify which chart migration caused the issue, leading to slower debugging and resolution times. ###### Suggested change Include the name of the failing chart migration in the error message: try: for migration in [MigrateAreaChart, MigrateBarChart, ...]: migration.upgrade(session) session.commit() except Exception as e: session.rollback() raise Exception(f"Error upgrading {migration.__name__}: {e}") from e </details> ###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews. <!--- korbi internal id:b25bfc9b-b826-40c5-a200-1f314ef16955 --> ########## superset/examples/birth_names.py: ########## @@ -239,7 +239,7 @@ def create_slices(tbl: SqlaTable) -> tuple[list[Slice], list[Slice]]: Slice( **slice_kwargs, slice_name="Genders by State", - viz_type="dist_bar", + viz_type="echarts_timeseries_bar", Review Comment: ### Incorrect Chart Type for State Data <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The chart's visualization type 'echarts_timeseries_bar' is incorrect for displaying gender distribution by state, which is not a time series data. ###### Why this matters Using a time series visualization for non-time series data will result in incorrect data representation and could mislead users looking at state-wise gender distribution. ###### Suggested change Replace 'echarts_timeseries_bar' with 'echarts_bar' or 'dist_bar' which are more appropriate for displaying categorical state-wise comparisons. </details> ###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews. <!--- korbi internal id:1fd30521-0d83-48bd-94db-9f507c13aad6 --> ########## superset-frontend/src/explore/components/SaveModal.tsx: ########## @@ -132,7 +132,7 @@ class SaveModal extends Component<SaveModalProps, SaveModalState> { }); } } catch (error) { - this.props.actions.addDangerToast( + this.props.addDangerToast( t('An error occurred while loading dashboard information.'), ); Review Comment: ### Lost Error Context in Toast Message <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The error catch block discards the error details when loading dashboard information. ###### Why this matters Loss of error context makes debugging production issues more difficult since the actual cause of the failure is not captured. ###### Suggested change Include error details in the toast message: `t('An error occurred while loading dashboard information: ${error.message}')` </details> ###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews. <!--- korbi internal id:27656e28-cdcf-4ecf-84f0-f94ec1b41f5b --> -- 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]
