Skip to content

Commit

Permalink
addressed comment
Browse files Browse the repository at this point in the history
  • Loading branch information
yashvardhan-n committed Dec 13, 2024
1 parent 3affc0d commit 39c3a66
Show file tree
Hide file tree
Showing 6 changed files with 224 additions and 146 deletions.
5 changes: 4 additions & 1 deletion src/main/custom-chart-context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import {
VisualEditorDefinitionSetter,
VisualPropEditorDefinition,
} from '../types/visual-prop.types';
import { setLocaleBasedStringFormats } from '../utils/number-formatting/number-formatting-utils';
import { create } from './logger';
import {
globalThis,
Expand Down Expand Up @@ -1020,7 +1021,9 @@ export class CustomChartContext {
this.containerEl = payload.containerElSelector
? document.querySelector(payload.containerElSelector)
: null;

setLocaleBasedStringFormats(
this.appConfig.dateFormatsConfig?.tsLocaleBasedStringsFormats,
);
return this.publishChartContextPropsToHost();
};

Expand Down
9 changes: 6 additions & 3 deletions src/utils/globalize-Initializer/globalize-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ import supplemental from 'cldr-data/supplemental/likelySubtags.json';
import enpluralJson from 'cldr-data/supplemental/plurals.json';
import Globalize from 'globalize';
import _ from 'lodash';
import { create } from '../../main/logger';

const logger = create('globalize-initializer');

let currentLocale: string;
let currentCurrencyFormat: any;
Expand Down Expand Up @@ -54,7 +57,7 @@ export const getDefaultCurrencyCode = (): string => {
countryCode.toUpperCase()
];
if (!regionData || regionData.length === 0) {
console.warn('No currency data found for country:', countryCode);
logger.warn('No currency data found for country:', countryCode);
return 'GBP';
}
return Object.keys(regionData[regionData.length - 1])[0];
Expand Down Expand Up @@ -170,7 +173,7 @@ export function formatNumberSafely<
const formattedNumber = formatter(num);
return formattedNumber;
} catch (e) {
console.error('Error formatting pattern: ', format, num, e);
logger.error('Error formatting pattern: ', format, num, e);
if (Math.abs(num) < 1e-7) {
return '0';
}
Expand Down Expand Up @@ -206,7 +209,7 @@ export const validateNumberFormat = (format: string): boolean => {
...({ raw: sanitizeFormat(format) } as any),
})(123); // Test the formatter with a dummy value
} catch (e) {
console.error('Invalid number format:', format, e);
logger.error('Invalid number format:', format, e);
return false;
}
return true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@ import {
formatNegativeValue,
formatSpecialDataValue,
getAutoUnit,
getLocaleBasedStringFormats,
getLocaleName,
mapFormatterConfig,
PROTO_TO_UNITS,
setLocaleBasedStringFormats,
UNITS_TO_DIVIDING_FACTOR,
} from './formatting-utils';
} from './number-formatting-utils';

describe('formatNegativeValue', () => {
it('should format with prefix dash', () => {
Expand Down Expand Up @@ -259,6 +261,11 @@ describe('formatSpecialDataValue', () => {
const result = formatSpecialDataValue('Test');
expect(result).toBeNull();
});

it('should return Infinity for Infinity', () => {
const result = formatSpecialDataValue(Infinity);
expect(result).toBe('Infinity');
});
});

describe('Unit constants', () => {
Expand All @@ -274,3 +281,41 @@ describe('Unit constants', () => {
expect(PROTO_TO_UNITS[3]).toBe(Unit.Million);
});
});

describe('Locale String Formats', () => {
const strings: Record<string, string> = {
NULL_VALUE_PLACEHOLDER_LABEL: '{Null}',
EMPTY_VALUE_PLACEHOLDER_LABEL: '{Empty}',
};

it('should return an default object if setLocaleBasedStringFormats is not called', () => {
const result = getLocaleBasedStringFormats();
expect(result).toEqual(strings);
});

it('should set and get locale-based string formats correctly', () => {
const testLocaleStrings = {
greeting: 'Hello',
farewell: 'Goodbye',
};

setLocaleBasedStringFormats(testLocaleStrings);

const result = getLocaleBasedStringFormats();

expect(result).toEqual(testLocaleStrings);
});

it('should update the string formats when setLocaleBasedStringFormats is called multiple times', () => {
const firstSet = { greeting: 'Hola' };
const secondSet = { greeting: 'Bonjour' };

// Set first locale-based string formats
setLocaleBasedStringFormats(firstSet);
expect(getLocaleBasedStringFormats()).toEqual(firstSet);

// Set second locale-based string formats
setLocaleBasedStringFormats(secondSet);
expect(getLocaleBasedStringFormats()).toEqual(secondSet);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,21 @@ export const PROTO_TO_UNITS = {
/**
* Default strings for placeholders.
*/
const strings = {
let strings: Record<string, string> = {
NULL_VALUE_PLACEHOLDER_LABEL: '{Null}',
EMPTY_VALUE_PLACEHOLDER_LABEL: '{Empty}',
};

export const setLocaleBasedStringFormats = (
tsLocaleBasedStringsFormats?: Record<string, string>,
) => {
if (tsLocaleBasedStringsFormats) {
strings = tsLocaleBasedStringsFormats;
}
};

export const getLocaleBasedStringFormats = () => strings;

export const UNITS_TO_SUFFIX: Record<Unit, string> = {
[Unit.None]: '',
[Unit.Thousands]: 'K',
Expand Down Expand Up @@ -260,7 +270,7 @@ export function formatSpecialDataValue(value: any) {
return strings.EMPTY_VALUE_PLACEHOLDER_LABEL;
}
if (value instanceof Array) {
if (!value.length || value[0] === null || value[0] === undefined) {
if (_.isEmpty(value) || _.isNil(value[0])) {
return strings.NULL_VALUE_PLACEHOLDER_LABEL;
}

Expand Down
43 changes: 23 additions & 20 deletions src/utils/number-formatting/number-formatting.spec.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { create, LogLevel, logMethods } from '../../main/logger';
import {
ColumnFormat,
CurrencyFormatType,
Expand All @@ -16,6 +17,10 @@ import {
getFormattedValue,
} from './number-formatting';

jest.mock('../../main/util', () => ({
getQueryParam: jest.fn().mockReturnValue('true'),
}));

describe('formatCurrencyWithCustomPattern', () => {
beforeAll(() => {
initGlobalize('en-US');
Expand Down Expand Up @@ -203,38 +208,36 @@ describe('getFormattedValue', () => {
category: CategoryType.Custom,
customFormatConfig: { format: 'invalid-format' },
};
const consoleSpy = jest
.spyOn(console, 'error')
.mockImplementationOnce(() => undefined);
const error = jest.fn();
logMethods[LogLevel.ERROR] = error;
const logger = create('TestLogger');
logger.error = error;

getFormattedValue(12345.678, formatConfig, {} as ColumnFormat);
expect(consoleSpy).toHaveBeenCalledWith(
'Invalid custom format config passed:',
formatConfig,
);
consoleSpy.mockRestore();
jest.resetAllMocks();
expect(logger.error).toHaveBeenCalled();

jest.restoreAllMocks();
});

test('handles currency formatting failure gracefully', () => {
test('handles currency formatting failure', () => {
const formatConfig: FormatConfig = {
category: CategoryType.Currency,
};
const consoleSpy = jest
.spyOn(console, 'error')
.mockImplementationOnce(() => undefined);
jest.spyOn(
globalizeUtils,
'globalizeCurrencyFormatter',
).mockImplementationOnce(() => {
throw new Error('Currency format error');
});

const error = jest.fn();
logMethods[LogLevel.ERROR] = error;
const logger = create('TestLogger');
logger.error = error;

getFormattedValue(12345.678, formatConfig, {} as ColumnFormat);
expect(consoleSpy).toHaveBeenCalledWith(
'Corrupted format config passed, formatting using default config',
formatConfig,
new Error('Currency format error'),
);
consoleSpy.mockRestore();
jest.resetAllMocks();
expect(logger.error).toHaveBeenCalled();

jest.restoreAllMocks();
});
});
Loading

0 comments on commit 39c3a66

Please sign in to comment.