codeant-ai-for-open-source[bot] commented on code in PR #39760:
URL: https://github.com/apache/superset/pull/39760#discussion_r3613165460
##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts:
##########
@@ -230,39 +298,167 @@ export const stripGeomColumnFromLabelMap = (
* @returns query data without geom column.
*/
export const stripGeomColumnFromQueryData = (
- queryData: any,
+ queryData: TimeseriesChartDataResponseResult,
geomColumn: string,
+ geomFormat: GeometryFormat,
) => {
const queryDataClone = {
...structuredClone(queryData),
- ...stripGeomFromColnamesAndTypes(queryData, geomColumn),
+ ...stripGeomFromColnamesAndTypes(queryData, geomColumn, geomFormat),
};
if (queryDataClone.label_map) {
queryDataClone.label_map = stripGeomColumnFromLabelMap(
queryData.label_map,
geomColumn,
+ geomFormat,
);
}
return queryDataClone;
};
+// copy of
+// superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
+export const formatSeriesName = (
+ name: DataRecordValue | undefined,
+ {
+ numberFormatter,
+ timeFormatter,
+ coltype,
+ }: {
+ numberFormatter?: ValueFormatter;
+ timeFormatter?: TimeFormatter;
+ coltype?: GenericDataType;
+ } = {},
+) => {
+ if (name === undefined || name === null) {
+ return NULL_STRING;
+ }
+ if (typeof name === 'boolean' || typeof name === 'bigint') {
+ return name.toString();
+ }
+ if (name instanceof Date || coltype === GenericDataType.Temporal) {
+ const normalizedName =
+ typeof name === 'string' ? normalizeTimestamp(name) : name;
+ const d =
+ normalizedName instanceof Date
+ ? normalizedName
+ : new Date(normalizedName);
+
+ return timeFormatter ? timeFormatter(d) : d.toISOString();
+ }
+ if (typeof name === 'number') {
+ return numberFormatter ? numberFormatter(name) : name.toString();
+ }
+ return name;
+};
+
+// copy of
+// superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
+export const extractGroupbyLabel = ({
+ datum = {},
+ groupby,
+ numberFormatter,
+ timeFormatter,
+ coltypeMapping = {},
+}: {
+ datum?: DataRecord;
+ groupby?: string[] | null;
+ numberFormatter?: NumberFormatter;
+ timeFormatter?: TimeFormatter;
+ coltypeMapping?: Record<string, GenericDataType>;
+}) =>
+ ensureIsArray(groupby)
+ .map(val =>
+ formatSeriesName(datum[val], {
+ numberFormatter,
+ timeFormatter,
+ ...(coltypeMapping[val] && { coltype: coltypeMapping[val] }),
+ }),
+ )
+ .join(', ');
+
+// copy of
+// superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
+export const getColtypesMapping = ({
+ coltypes = [],
+ colnames = [],
+}: Pick<ChartDataResponseResult, 'coltypes' | 'colnames'>): Record<
+ string,
+ GenericDataType
+> =>
+ colnames.reduce(
+ (accumulator, item, index) => ({ ...accumulator, [item]: coltypes[index]
}),
+ {},
+ );
+
+/**
+ * Reserve label colors for the chart.
+ *
+ * We call the CategoricalColorNamespace singleton to reserve
+ * label colors for the chart. This is necessary to ensure that
+ * we do not run into color collisions when rendering multiple
+ * charts in the same cartodiagram.
+ *
+ * TODO This only works in the context of a dashboard,
+ * since only there, the CategoricalColorNamespace singleton is being
used.
+ * In the explore view, label colors cannot be reserved without changing
+ * the overall color handling in superset.
+ *
+ * @param formData The formdata of the underlying chart
+ * @param dataByLocation The data grouped by location
+ * @param strippedQueryData The query data without the geom column
+ * @param sliceId The id of the chart slice
+ */
+export const reserveLabelColors = (
+ formData: Record<string, any>,
Review Comment:
**Suggestion:** Replace the `any` in the form data type with a specific
interface or a well-scoped typed record that reflects the expected fields.
[custom_rule]
**Severity Level:** Major โ ๏ธ
<details>
<summary><b>Why it matters? โญ </b></summary>
The rule forbids `any` types, and this line explicitly uses `Record<string,
any>`. The suggestion accurately identifies a real violation in the final file
state.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
=== .cursor/rules/dev-standard.mdc === (line 16)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=11a4ec25108b4fb1943c563249dea18e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=11a4ec25108b4fb1943c563249dea18e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts
**Line:** 413:413
**Comment:**
*Custom Rule: Replace the `any` in the form data type with a specific
interface or a well-scoped typed record that reflects the expected fields.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=f603892c72a5f0ce94a1d3963bb31e97fb70ad7c82787bb23c627feb4ad62d77&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=f603892c72a5f0ce94a1d3963bb31e97fb70ad7c82787bb23c627feb4ad62d77&reaction=dislike'>๐</a>
##########
superset-frontend/plugins/plugin-chart-cartodiagram/test/testData.ts:
##########
@@ -75,6 +81,112 @@ export const nonTimeSeriesChartData: any = [
},
];
Review Comment:
**Suggestion:** Replace the `any` annotation with a concrete array item type
that describes the shape of each data object. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The custom rule forbids `any` types. This line explicitly annotates the
exported array as `any`, so the violation is real and present in the current
file.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
=== .cursor/rules/dev-standard.mdc === (line 16)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4c198f98adc94c6195ad03936ea9ce29&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4c198f98adc94c6195ad03936ea9ce29&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-cartodiagram/test/testData.ts
**Line:** 83:83
**Comment:**
*Custom Rule: Replace the `any` annotation with a concrete array item
type that describes the shape of each data object.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=873b0de9e0905caea957f042d07e16f89c2afd09d5ebdab4b738745f442719d2&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=873b0de9e0905caea957f042d07e16f89c2afd09d5ebdab4b738745f442719d2&reaction=dislike'>๐</a>
##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts:
##########
@@ -230,39 +298,167 @@ export const stripGeomColumnFromLabelMap = (
* @returns query data without geom column.
*/
export const stripGeomColumnFromQueryData = (
- queryData: any,
+ queryData: TimeseriesChartDataResponseResult,
geomColumn: string,
+ geomFormat: GeometryFormat,
) => {
const queryDataClone = {
...structuredClone(queryData),
- ...stripGeomFromColnamesAndTypes(queryData, geomColumn),
+ ...stripGeomFromColnamesAndTypes(queryData, geomColumn, geomFormat),
};
if (queryDataClone.label_map) {
queryDataClone.label_map = stripGeomColumnFromLabelMap(
queryData.label_map,
geomColumn,
+ geomFormat,
);
}
return queryDataClone;
};
+// copy of
+// superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
+export const formatSeriesName = (
+ name: DataRecordValue | undefined,
+ {
+ numberFormatter,
+ timeFormatter,
+ coltype,
+ }: {
+ numberFormatter?: ValueFormatter;
+ timeFormatter?: TimeFormatter;
+ coltype?: GenericDataType;
+ } = {},
+) => {
+ if (name === undefined || name === null) {
+ return NULL_STRING;
+ }
+ if (typeof name === 'boolean' || typeof name === 'bigint') {
+ return name.toString();
+ }
+ if (name instanceof Date || coltype === GenericDataType.Temporal) {
+ const normalizedName =
+ typeof name === 'string' ? normalizeTimestamp(name) : name;
+ const d =
+ normalizedName instanceof Date
+ ? normalizedName
+ : new Date(normalizedName);
+
+ return timeFormatter ? timeFormatter(d) : d.toISOString();
+ }
+ if (typeof name === 'number') {
+ return numberFormatter ? numberFormatter(name) : name.toString();
+ }
+ return name;
+};
+
+// copy of
+// superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
+export const extractGroupbyLabel = ({
+ datum = {},
+ groupby,
+ numberFormatter,
+ timeFormatter,
+ coltypeMapping = {},
+}: {
+ datum?: DataRecord;
+ groupby?: string[] | null;
+ numberFormatter?: NumberFormatter;
+ timeFormatter?: TimeFormatter;
+ coltypeMapping?: Record<string, GenericDataType>;
+}) =>
+ ensureIsArray(groupby)
+ .map(val =>
+ formatSeriesName(datum[val], {
+ numberFormatter,
+ timeFormatter,
+ ...(coltypeMapping[val] && { coltype: coltypeMapping[val] }),
+ }),
+ )
+ .join(', ');
+
+// copy of
+// superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
+export const getColtypesMapping = ({
+ coltypes = [],
+ colnames = [],
+}: Pick<ChartDataResponseResult, 'coltypes' | 'colnames'>): Record<
+ string,
+ GenericDataType
+> =>
+ colnames.reduce(
+ (accumulator, item, index) => ({ ...accumulator, [item]: coltypes[index]
}),
+ {},
+ );
+
+/**
+ * Reserve label colors for the chart.
+ *
+ * We call the CategoricalColorNamespace singleton to reserve
+ * label colors for the chart. This is necessary to ensure that
+ * we do not run into color collisions when rendering multiple
+ * charts in the same cartodiagram.
+ *
+ * TODO This only works in the context of a dashboard,
+ * since only there, the CategoricalColorNamespace singleton is being
used.
+ * In the explore view, label colors cannot be reserved without changing
+ * the overall color handling in superset.
+ *
+ * @param formData The formdata of the underlying chart
+ * @param dataByLocation The data grouped by location
+ * @param strippedQueryData The query data without the geom column
+ * @param sliceId The id of the chart slice
+ */
+export const reserveLabelColors = (
+ formData: Record<string, any>,
+ dataByLocation: LocationConfigMapping,
+ strippedQueryData: any,
Review Comment:
**Suggestion:** Replace the untyped query data parameter with a specific
Superset response type (or a narrowed subset type) to preserve type safety.
[custom_rule]
**Severity Level:** Major โ ๏ธ
<details>
<summary><b>Why it matters? โญ </b></summary>
The rule forbids `any` types, and this parameter is explicitly typed as
`any` in the final code. This is a direct, real violation of the custom rule.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
=== .cursor/rules/dev-standard.mdc === (line 16)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a48c805da9394036a0e05efbf3019f4e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a48c805da9394036a0e05efbf3019f4e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts
**Line:** 415:415
**Comment:**
*Custom Rule: Replace the untyped query data parameter with a specific
Superset response type (or a narrowed subset type) to preserve type safety.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=27efc9b599991f7ae51f0c00dd2a0b4e3a25a31003f139f1551cb208122daf83&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=27efc9b599991f7ae51f0c00dd2a0b4e3a25a31003f139f1551cb208122daf83&reaction=dislike'>๐</a>
##########
superset-frontend/plugins/plugin-chart-cartodiagram/test/testData.ts:
##########
@@ -75,6 +81,112 @@ export const nonTimeSeriesChartData: any = [
},
];
+export const nonTimeSeriesWkbChartData: any = [
+ {
+ geom: wkbGeom1,
+ my_value: 'apple',
+ my_count: 347,
+ },
+ {
+ geom: wkbGeom1,
+ my_value: 'apple',
+ my_count: 360,
+ },
+ {
+ geom: wkbGeom1,
+ my_value: 'lemon',
+ my_count: 335,
+ },
+ {
+ geom: wkbGeom1,
+ my_value: 'lemon',
+ my_count: 333,
+ },
+ {
+ geom: wkbGeom1,
+ my_value: 'lemon',
+ my_count: 353,
+ },
+ {
+ geom: wkbGeom1,
+ my_value: 'lemon',
+ my_count: 359,
+ },
+ {
+ geom: wkbGeom2,
+ my_value: 'lemon',
+ my_count: 347,
+ },
+ {
+ geom: wkbGeom2,
+ my_value: 'apple',
+ my_count: 335,
+ },
+ {
+ geom: wkbGeom2,
+ my_value: 'apple',
+ my_count: 356,
+ },
+ {
+ geom: wkbGeom2,
+ my_value: 'banana',
+ my_count: 218,
+ },
+];
+
Review Comment:
**Suggestion:** Replace the `any` annotation with a specific array item type
for the WKT dataset objects. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The custom rule forbids `any` types. This export is annotated with `any`, so
it is a genuine violation in the final file state.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
=== .cursor/rules/dev-standard.mdc === (line 16)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8f957ef984c1476f9ffef6321b40d71b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8f957ef984c1476f9ffef6321b40d71b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-cartodiagram/test/testData.ts
**Line:** 136:136
**Comment:**
*Custom Rule: Replace the `any` annotation with a specific array item
type for the WKT dataset objects.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=a766dd189232f82f5c9f2e4f5daffc745865734b7861ea8bab505803f0c971d2&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=a766dd189232f82f5c9f2e4f5daffc745865734b7861ea8bab505803f0c971d2&reaction=dislike'>๐</a>
##########
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/mapUtil.test.ts:
##########
@@ -22,10 +22,14 @@ import OSM from 'ol/source/OSM.js';
import TileLayer from 'ol/layer/Tile.js';
import View from 'ol/View.js';
import { ChartConfig } from '../../src/types';
-import { fitMapToCharts } from '../../src/util/mapUtil';
+import {
+ fitMapToData,
+ wkbToGeoJSON,
+ wktToGeoJSON,
+} from '../../src/util/mapUtil';
describe('mapUtil', () => {
- describe('fitMapToCharts', () => {
+ describe('fitMapToData', () => {
Review Comment:
**Suggestion:** Replace this nested suite block with a `test()` case to
follow the no-`describe()` testing rule. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The custom rule explicitly says to use test() instead of describe(). This
file contains a describe() wrapper for the fitMapToData group, so the
suggestion correctly identifies a real rule violation.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
=== .github/copilot-instructions.md === (line 43)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b706f6035b3c4566ba6e5c13be10dc34&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b706f6035b3c4566ba6e5c13be10dc34&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/mapUtil.test.ts
**Line:** 32:32
**Comment:**
*Custom Rule: Replace this nested suite block with a `test()` case to
follow the no-`describe()` testing rule.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=e3caf4a926f00df4ce505f2cc857b75db463192a1ecd2c840a1454654f20a05c&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=e3caf4a926f00df4ce505f2cc857b75db463192a1ecd2c840a1454654f20a05c&reaction=dislike'>๐</a>
##########
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/mapUtil.test.ts:
##########
@@ -106,11 +110,54 @@ describe('mapUtil', () => {
});
// should set center
- fitMapToCharts(olMap, chartConfig);
+ fitMapToData(olMap, chartConfig);
const updatedCenter = olMap.getView().getCenter();
expect(initialCenter).not.toEqual(updatedCenter);
});
});
+
+ describe('wkbToGeoJSON', () => {
+ test('converts WKB to GeoJSON', () => {
+ const wkb = '0101000020E610000000000000000020400000000000804A40';
+ const geoJSON = wkbToGeoJSON(wkb);
+ expect(geoJSON).toEqual({
+ type: 'Feature',
+ geometry: {
+ type: 'Point',
+ coordinates: [8, 53],
+ },
+ properties: null,
+ });
+ });
+ });
+
+ describe('wktToGeoJSON', () => {
Review Comment:
**Suggestion:** Remove this `describe()` wrapper and keep the related
assertions as standalone `test()` blocks. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This also directly violates the rule requiring test() instead of describe().
The existing code wraps the WKT tests in a describe() block, so the suggestion
is valid.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
=== .github/copilot-instructions.md === (line 43)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4f2d004332e94ab39215b980082c0582&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4f2d004332e94ab39215b980082c0582&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/mapUtil.test.ts
**Line:** 136:136
**Comment:**
*Custom Rule: Remove this `describe()` wrapper and keep the related
assertions as standalone `test()` blocks.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=08fa5cb84a6b98f319b8295835a1e9eab451b30527190538d560f5ce85e301ad&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=08fa5cb84a6b98f319b8295835a1e9eab451b30527190538d560f5ce85e301ad&reaction=dislike'>๐</a>
##########
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/transformPropsUtil.test.ts:
##########
@@ -25,24 +26,86 @@ import {
groupByLocationGenericX,
stripGeomFromColnamesAndTypes,
stripGeomColumnFromLabelMap,
+ getWkbColumns,
+ getWktColumns,
} from '../../src/util/transformPropsUtil';
import {
nonTimeSeriesChartData,
groupedTimeseriesChartData,
geom1,
geom2,
groupedTimeseriesLabelMap,
+ nonTimeSeriesWkbChartData,
+ nonTimeSeriesWktChartData,
} from '../testData';
describe('transformPropsUtil', () => {
const groupedTimeseriesParams = {
x_axis: 'mydate',
};
+ const sliceId = 1;
+
const groupedTimeseriesQueryData = {
label_map: groupedTimeseriesLabelMap,
};
+ describe('getWkbColumns', () => {
Review Comment:
**Suggestion:** Replace this nested suite wrapper with `test()` cases so the
new tests follow the flat testing pattern. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is a test file and the new coverage is wrapped in a `describe()` block,
which violates the stated rule to use `test()` instead of `describe()`.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
=== .github/copilot-instructions.md === (line 43)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4733aba8b3604ab5b04e1e777e7c1a49&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4733aba8b3604ab5b04e1e777e7c1a49&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/transformPropsUtil.test.ts
**Line:** 53:53
**Comment:**
*Custom Rule: Replace this nested suite wrapper with `test()` cases so
the new tests follow the flat testing pattern.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=0700647c86d61f9e6f9ea98da858e1c0ca7ccbea5e89c46575d5bc51dbf35558&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=0700647c86d61f9e6f9ea98da858e1c0ca7ccbea5e89c46575d5bc51dbf35558&reaction=dislike'>๐</a>
##########
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/transformPropsUtil.test.ts:
##########
@@ -25,24 +26,86 @@ import {
groupByLocationGenericX,
stripGeomFromColnamesAndTypes,
stripGeomColumnFromLabelMap,
+ getWkbColumns,
+ getWktColumns,
} from '../../src/util/transformPropsUtil';
import {
nonTimeSeriesChartData,
groupedTimeseriesChartData,
geom1,
geom2,
groupedTimeseriesLabelMap,
+ nonTimeSeriesWkbChartData,
+ nonTimeSeriesWktChartData,
} from '../testData';
describe('transformPropsUtil', () => {
const groupedTimeseriesParams = {
x_axis: 'mydate',
};
+ const sliceId = 1;
+
const groupedTimeseriesQueryData = {
label_map: groupedTimeseriesLabelMap,
};
+ describe('getWkbColumns', () => {
+ test('gets the WKB columns', () => {
+ const wkbCol = '0101000020E610000000000000000020400000000000804A40';
+ const columns = ['foo', 'bar', wkbCol];
+
+ const result = getWkbColumns(columns);
+ expect(result).toHaveLength(1);
+ expect(result[0]).toEqual(2);
+ });
+
+ test('gets multiple WKB columns', () => {
+ const wkbCol = '0101000020E610000000000000000020400000000000804A40';
+ const columns = ['foo', 'bar', wkbCol, wkbCol];
+
+ const result = getWkbColumns(columns);
+ expect(result).toHaveLength(2);
+ expect(result[0]).toEqual(2);
+ expect(result[1]).toEqual(3);
+ });
+
+ test('returns empty array when no WKB column is included', () => {
+ const columns = ['foo', 'bar'];
+
+ const result = getWkbColumns(columns);
+ expect(result).toHaveLength(0);
+ });
+ });
+
+ describe('getWktColumns', () => {
Review Comment:
**Suggestion:** Convert this suite declaration into `test()` usage to comply
with the no-`describe()` testing rule. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This new suite wrapper uses `describe()` in a test file, which matches the
custom rule requiring `test()` instead of `describe()`.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
=== .github/copilot-instructions.md === (line 43)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cc46bc6109c84f8ab493878117005e3e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=cc46bc6109c84f8ab493878117005e3e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/transformPropsUtil.test.ts
**Line:** 81:81
**Comment:**
*Custom Rule: Convert this suite declaration into `test()` usage to
comply with the no-`describe()` testing rule.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=33c9080968d441f475c4cc213f9ac5554d9890b6b39f4e2180e5dccf57498b98&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=33c9080968d441f475c4cc213f9ac5554d9890b6b39f4e2180e5dccf57498b98&reaction=dislike'>๐</a>
##########
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/mapUtil.test.ts:
##########
@@ -106,11 +110,54 @@ describe('mapUtil', () => {
});
// should set center
- fitMapToCharts(olMap, chartConfig);
+ fitMapToData(olMap, chartConfig);
const updatedCenter = olMap.getView().getCenter();
expect(initialCenter).not.toEqual(updatedCenter);
});
});
+
+ describe('wkbToGeoJSON', () => {
Review Comment:
**Suggestion:** Convert this `describe()` block into one or more `test()`
calls directly under the parent scope. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is a real violation of the no-describe rule because the test file uses
a describe() block for the wkbToGeoJSON section instead of standalone test()
calls.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
=== .github/copilot-instructions.md === (line 43)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c0e729fb7954432eac485b33e558e408&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c0e729fb7954432eac485b33e558e408&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/mapUtil.test.ts
**Line:** 121:121
**Comment:**
*Custom Rule: Convert this `describe()` block into one or more `test()`
calls directly under the parent scope.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=2c320dc0124e07978b1194644f2c5e1dbc8650991b46c72e6b3df79e67fd781a&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=2c320dc0124e07978b1194644f2c5e1dbc8650991b46c72e6b3df79e67fd781a&reaction=dislike'>๐</a>
--
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]
