[GitHub] [superset] villebro commented on a diff in pull request #24517: feat: Implement currencies formatter for saved metrics

["villebro (via GitHub)" <gi...@apache.org>]

villebro commented on code in PR #24517:
URL: https://github.com/apache/superset/pull/24517#discussion_r1243162274


##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/buildCustomFormatters.ts:
##########
@@ -0,0 +1,74 @@
+import {
+  Currency,
+  CurrencyFormatter,
+  ensureIsArray,
+  getNumberFormatter,
+  isDefined,
+  isSavedMetric,
+  NumberFormatter,
+  QueryFormMetric,
+} from '@superset-ui/core';
+
+export const buildCustomFormatters = (
+  metrics: QueryFormMetric | QueryFormMetric[] | undefined,
+  currencyFormats: Record<string, Currency>,
+  columnFormats: Record<string, string>,
+  d3Format: string | undefined,
+  labelMap: Record<string, string[]> = {},
+  seriesNames: string[] = [],
+) => {
+  const metricsArray = ensureIsArray(metrics);
+  if (metricsArray.length === 1) {
+    const metric = metricsArray[0];
+    if (isSavedMetric(metric)) {
+      return {
+        [metric]: currencyFormats[metric]
+          ? new CurrencyFormatter({
+              d3Format: columnFormats[metric] ?? d3Format,
+              currency: currencyFormats[metric],
+            })
+          : getNumberFormatter(columnFormats[metric] ?? d3Format),
+      };
+    }
+    return {};
+  }
+  return seriesNames.reduce((acc, name) => {
+    if (!isDefined(name)) {
+      return acc;
+    }
+
+    const metricName = labelMap[name]?.[0];
+    const isSaved =
+      metricName &&
+      // string equality checks if it is a saved metric
+      metricsArray?.some(metric => metric === metricName);
+    const actualD3Format = isSaved
+      ? columnFormats[metricName] ?? d3Format
+      : d3Format;
+    if (isSaved && currencyFormats[metricName]) {
+      return {
+        ...acc,
+        [name]: new CurrencyFormatter({
+          d3Format: actualD3Format,
+          currency: currencyFormats[metricName],
+        }),
+      };
+    }
+    return {
+      ...acc,
+      [name]: getNumberFormatter(actualD3Format),
+    };
+  }, {});
+};
+
+export const getCustomFormatter = (
+  customFormatters: Record<string, NumberFormatter | CurrencyFormatter>,
+  metrics: QueryFormMetric | QueryFormMetric[] | undefined,
+  key?: string,
+) => {

Review Comment:
   same here



##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:
##########
@@ -141,7 +148,17 @@ export default function transformProps(
   };
   const refs: Refs = {};
   const colorFn = CategoricalColorNamespace.getScale(colorScheme as string);
-  const numberFormatter = getNumberFormatter(numberFormat);
+  const numberFormatter =
+    getCustomFormatter(
+      buildCustomFormatters(
+        metric,
+        currencyFormats,
+        columnFormats,
+        numberFormat,
+      ),
+      metric,
+    ) ?? getNumberFormatter(numberFormat);

Review Comment:
   At the risk of over-abstraction, would it make sense to have yet one more helper that simplifies this call chain? This code block seems to be repeated a few times so abstracting it would make it slightly more DRY and less heavy on the eyes. Something like
   ```ts
     const numberFormatter =
       getFormatter(
         metric,
         currencyFormats,
         columnFormats,
         numberFormat,
       );
   ```



##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/buildCustomFormatters.ts:
##########
@@ -0,0 +1,74 @@
+import {
+  Currency,
+  CurrencyFormatter,
+  ensureIsArray,
+  getNumberFormatter,
+  isDefined,
+  isSavedMetric,
+  NumberFormatter,
+  QueryFormMetric,
+} from '@superset-ui/core';
+
+export const buildCustomFormatters = (
+  metrics: QueryFormMetric | QueryFormMetric[] | undefined,
+  currencyFormats: Record<string, Currency>,
+  columnFormats: Record<string, string>,
+  d3Format: string | undefined,
+  labelMap: Record<string, string[]> = {},
+  seriesNames: string[] = [],
+) => {

Review Comment:
   It seems the return type is `{ [key: string]: CurrencyFormatter | NumberFormatter }`, could we add that explicitly here to make it easier to read?



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org