aminghadersohi commented on PR #40351: URL: https://github.com/apache/superset/pull/40351#issuecomment-4595244388
Addressed all findings from the review (commit e5e4a8bc): **HIGH-1** — Both `create_layer_annotation` and `update_layer_annotation` now return a structured `CreateLayerAnnotationResponse`/`UpdateLayerAnnotationResponse` with `error` set instead of re-raising. `logger.exception` is called for traceback capture. **MEDIUM-1** — Added `# deferred import — avoids circular import at module load time` comment to the inline import blocks in both tools. **MEDIUM-2** — Added `test_create_layer_annotation_unexpected_exception` and `test_update_layer_annotation_unexpected_exception` verifying the structured error response for `RuntimeError`. **MEDIUM-3** — Added `test_create_layer_annotation_end_before_start_raises_invalid_error` that passes `end_dttm < start_dttm` and mocks the command to raise `AnnotationInvalidError`, asserting the tool returns a structured error response. **NIT-1 / bug** — Extracted `_validate_json_metadata_field` as a module-level helper and replaced both `validate_json_metadata` validators to delegate to it. Also fixed a latent `NameError` in `UpdateLayerAnnotationRequest` which was calling `json.loads` (undefined) instead of `json_utils.loads`. **NIT-2** — Acknowledged as a schema design observation; not changed since the behavior is correct. **NIT-3** — `_build_update_properties` is covered transitively by `test_update_layer_annotation_only_provided_fields_forwarded` and `test_update_layer_annotation_clear_nullable_fields`; skipping a separate isolation test to avoid over-testing a small private helper. -- 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]
