Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delimiter Added in case of Number format #6

Merged
merged 7 commits into from Dec 23, 2021

Conversation

ravikant-paudel
Copy link
Contributor

@ravikant-paudel ravikant-paudel commented Dec 15, 2021

heading('With Empty Delimiter');
print('123456 -> ${currencyFormatWithNoDelimiter.format(123456)}');
print('1234 -> ${currencyFormatWithNoDelimiter.format(1234)}');
print('12345 -> ${currencyFormatWithNoDelimiter.format(12345)}');
print('123456 -> ${currencyFormatWithNoDelimiter.format(123456)}');
print('123456789.6548 -> ${delimiterWithDecimal.format(123456789.6548)}');

heading('With Delimiter');
print('123456 -> ${currencyFormatWithDelimiter.format(123456)}');
print('1234 -> ${currencyFormatWithDelimiter.format(1234)}');
print('12345 -> ${currencyFormatWithDelimiter.format(12345)}');
print('123456 -> ${currencyFormatWithDelimiter.format(123456)}');
print( '1234567.891 -> ${currencyFormatWithDelimiterAndDecimal.format(1234567.891)}');

image

Copy link
Owner

@sarbagyastha sarbagyastha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution. I've requested some changes, please go though it once.

Comment on lines 42 to 56
var currencyFormatWithDelimiterAndDecimal = NepaliNumberFormat(
decimalDigits: 2,
);
var currencyFormatWithDelimiter = NepaliNumberFormat(
symbol: 'Rs.',
);
var currencyFormatWithNoDelimiter = NepaliNumberFormat(
symbol: 'Rs.',
delimiter: '',
);
var delimiterWithDecimal = NepaliNumberFormat(
decimalDigits: 2,
delimiter: '',
);

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They don't seem to currency format as monetary is false by default. Please rename it to numberFormat.

Also, example for on with delimiter and another without it should be enough.

Comment on lines 74 to 86
heading('With Empty Delimiter');
print('1234 -> ${currencyFormatWithNoDelimiter.format(1234)}');
print('12345 -> ${currencyFormatWithNoDelimiter.format(12345)}');
print('123456 -> ${currencyFormatWithNoDelimiter.format(123456)}');
print('123456789.6548 -> ${delimiterWithDecimal.format(123456789.6548)}');

heading('With Delimiter');
print('1234 -> ${currencyFormatWithDelimiter.format(1234)}');
print('12345 -> ${currencyFormatWithDelimiter.format(12345)}');
print('123456 -> ${currencyFormatWithDelimiter.format(123456)}');
print(
'1234567.891 -> ${currencyFormatWithDelimiterAndDecimal.format(1234567.891)}');

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only two types of example should be enough

/// 2000(delimiter = '') -> 2000
/// 2000(delimiter = '.') -> 2.000
///
/// Default value ','.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default value is ','

@@ -229,15 +239,15 @@ class NepaliNumberFormat {
if (hideDecimal) {
return '${localizedNum[0]},${localizedNum.substring(1)}';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You missed adding delimiter here.

The case can be verified by setting includeDecimalIfZero: false and having number with all zeros after decimal point.

Comment on lines 128 to 156
group('hide comma tests:', () {
test(
'default is english',
() => expect(NepaliNumberFormat(delimiter: '').format(1234), '1234'),
);
test(
'formats in nepali',
() => expect(
NepaliNumberFormat(language: Language.nepali, delimiter: '')
.format(1234),
'१२३४',
),
);
});

group('hide comma tests:', () {
test(
'default is english',
() => expect(NepaliNumberFormat(delimiter: '*').format(1234), '1*234'),
);
test(
'formats in nepali',
() => expect(
NepaliNumberFormat(language: Language.nepali, delimiter: '*')
.format(1234),
'१*२३४',
),
);
});
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The group and it's test has same name, but it's doing different things.

@ravikant-paudel ravikant-paudel changed the title Hide comma Added in case of Number format Delimiter Added in case of Number format Dec 17, 2021
Copy link
Owner

@sarbagyastha sarbagyastha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sarbagyastha sarbagyastha merged commit c8cba4e into sarbagyastha:master Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants