tbonelee commented on code in PR #5257:
URL: https://github.com/apache/zeppelin/pull/5257#discussion_r3298212746
##########
zeppelin-web-angular/projects/zeppelin-react/src/components/visualizations/TableVisualization.tsx:
##########
@@ -75,72 +74,132 @@ export const TableVisualization = ({ result, config }:
TableVisualizationProps)
y: parseFloat(row[1] || '0') || 0
}));
- let chart: Column | Line | Pie | Scatter | null = null;
+ chartRef.current.innerHTML = '';
+
+ let chart: any = null;
Review Comment:
**praise:** Lint is clean now. `Chart | null` / `ChartConfiguration | null`
resolves both `no-explicit-any` errors, and the union still narrows per-case at
the `{ type: 'bar', ... }` literal assignment, so each case's dataset/options
shape stays type-checked. Verified locally with the project ruleset: errors 4 →
0 on this file.
**thought (non-blocking):** If you ever want tighter per-case typing,
`Chart<'bar'>` / `ChartConfiguration<'bar'>` etc. drop the union entirely and
narrow without relying on the assignment site. Current shape is good;
completely optional follow-up.
**nitpick (non-blocking):** Now that `container` is captured at effect
start, the `!container` check inside `import('chart.js/auto').then(...)` and
the `if (container)` guard in the cleanup are unreachable. `container` was
already non-null past the early return, and closure capture pins it for the
effect's lifetime. Safe to simplify to `if (cancelled) return;` and
`container.innerHTML = '';`. Pure tidy-up, no behavior change.
---
**praise:** lint이 깨끗하게 통과합니다. `Chart | null` / `ChartConfiguration | null`로
`no-explicit-any` 두 건이 해소됐고, union이어도 `{ type: 'bar', ... }` 리터럴 대입 지점에서
discriminant narrow가 일어나서 각 case의 dataset/options 모양은 그대로 타입체크됩니다. 프로젝트 룰셋 그대로
로컬에서 돌려보니 이 파일 에러 4 → 0 확인했어요.
**thought (non-blocking):** 더 타이트하게 가고 싶으시면 case별로 `Chart<'bar'>` /
`ChartConfiguration<'bar'>` 같은 제네릭을 박으면 union 없이 바로 narrow됩니다. 지금 형태도 충분히 좋아서
완전히 선택사항이에요.
**nitpick (non-blocking):** `container`를 effect 시작 시점에 캡처하신 덕에,
`import('chart.js/auto').then(...)` 안의 `!container` 체크와 cleanup의 `if
(container)` 가드는 더이상 도달할 일이 없습니다. early return을 지나면 `container`는 non-null이고,
effect 수명 동안 closure에 핀 됩니다. `if (cancelled) return;` / `container.innerHTML =
'';`로 줄여도 동작 동일합니다. 순수 정리 목적이라 선택사항이에요.
--
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]
