korbit-ai[bot] commented on code in PR #33072:
URL: https://github.com/apache/superset/pull/33072#discussion_r2036337342
##########
superset/app.py:
##########
@@ -79,14 +78,16 @@ class AppRootMiddleware:
def __init__(
self,
- wsgi_app: WSGIApplication,
+ wsgi_app: Any,
app_root: str,
):
self.wsgi_app = wsgi_app
self.app_root = app_root
def __call__(
- self, environ: WSGIEnvironment, start_response: StartResponse
+ self,
+ environ: Dict[str, Any],
+ start_response: Callable[[str, Any], Iterable[bytes]],
) -> Iterable[bytes]:
Review Comment:
### Overly Generic WSGI Type Hints <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The type hints for the WSGI middleware are overly generic using Dict[str,
Any] and Any types, making it harder to understand the expected interface.
###### Why this matters
Using generic type hints obscures the actual WSGI interface requirements and
makes it harder for developers to understand what data structures are expected.
###### Suggested change ∙ *Feature Preview*
```python
def __call__(
self,
environ: 'WSGIEnvironment',
start_response: 'StartResponse',
) -> Iterable[bytes]:
```
And re-add the removed imports:
```python
from wsgiref.types import StartResponse, WSGIEnvironment
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8ffa735d-3760-4a5f-8221-1e5167e85b85/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8ffa735d-3760-4a5f-8221-1e5167e85b85?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8ffa735d-3760-4a5f-8221-1e5167e85b85?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8ffa735d-3760-4a5f-8221-1e5167e85b85?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8ffa735d-3760-4a5f-8221-1e5167e85b85)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:be83f859-4b5a-4436-9081-bdb26b758e1b -->
[](be83f859-4b5a-4436-9081-bdb26b758e1b)
##########
superset/migrations/migration_utils.py:
##########
@@ -15,21 +15,32 @@
# specific language governing permissions and limitations
# under the License.
+import logging
+
from alembic.operations import Operations
naming_convention = {
"fk": "fk_%(table_name)s_%(column_0_name)s_%(referred_table_name)s",
"uq": "uq_%(table_name)s_%(column_0_name)s",
}
+logger = logging.getLogger(__name__)
+
def create_unique_constraint(
op: Operations, index_id: str, table_name: str, uix_columns: list[str]
) -> None:
- with op.batch_alter_table(
- table_name, naming_convention=naming_convention
- ) as batch_op:
- batch_op.create_unique_constraint(index_id, uix_columns)
+ # Run the DDL command in an autocommit block so a failure here
+ # won't abort the outer transaction.
+ try:
+ with op.get_context().autocommit_block():
+ with op.batch_alter_table(
+ table_name, naming_convention=naming_convention
+ ) as batch_op:
+ batch_op.create_unique_constraint(index_id, uix_columns)
+ except Exception as e:
Review Comment:
### Over-broad exception handling <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code uses a broad Exception catch which makes it unclear what specific
exceptions are being handled.
###### Why this matters
Catching specific exceptions helps other developers understand what error
scenarios are being handled and what could go wrong at this point in the code.
###### Suggested change ∙ *Feature Preview*
```python
# Replace with specific exceptions that could occur during constraint
creation
except (sqlalchemy.exc.OperationalError, sqlalchemy.exc.IntegrityError) as e:
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc640181-be5a-487b-8f93-8c8e6752e303/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc640181-be5a-487b-8f93-8c8e6752e303?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc640181-be5a-487b-8f93-8c8e6752e303?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc640181-be5a-487b-8f93-8c8e6752e303?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc640181-be5a-487b-8f93-8c8e6752e303)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:14c33031-7886-40c1-ac66-7f2acd35c572 -->
[](14c33031-7886-40c1-ac66-7f2acd35c572)
--
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]
