aminghadersohi commented on PR #43335: URL: https://github.com/apache/superset/pull/43335#issuecomment-5348269327
Thanks @bito-code-review — went through the three suggestions: **"OperationalError is not a StatementError subclass" (`base.py` L260/L351)** — It is, transitively, and that transitivity is the whole point of the comment: ```python >>> from sqlalchemy.exc import OperationalError, StatementError >>> issubclass(OperationalError, StatementError) True ``` MRO: `OperationalError → DatabaseError → DBAPIError → StatementError → SQLAlchemyError`. `DBAPIError` itself subclasses `StatementError`, so the `except StatementError` on the next line *does* catch `OperationalError` — which is exactly why the earlier `except OperationalError: raise` guard is required. The comment describes the relationship that governs the control flow, so it's accurate as written. Keeping as-is. **Move `FavStar` import to module level (`dashboards/dao_tests.py` L26)** — These fixtures import Superset models inside the fixture by design; the very next line (`from superset.models.dashboard import Dashboard`) does the same, as does every test's inline DAO import. The inline import keeps model registration ordered relative to the in-memory metadata setup, and hoisting only `FavStar` would be inconsistent with the surrounding file. Keeping as-is. The logic in all cases was already flagged as correct — these are comment/style notes, so no code change. -- 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]
