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

feat: add Ti.Local.parseDecimal() method #11683

Merged
merged 4 commits into from Jul 13, 2020

Conversation

jquick-axway
Copy link
Contributor

JIRA:
https://jira.appcelerator.org/browse/TIMOB-27874

Summary:

  • Parses a localized numeric string to a number.
  • Uses current locale or can provide optional locale for 2nd argument.
  • Returns NaN (not a number) if given an invalid string.
  • Can parse localized string returned by String.formatDecimal().
  • Expected to be used to parse value from numeric TextFields.

- Parses a localized numeric string to a number.
@jquick-axway jquick-axway added this to the 9.1.0 milestone May 5, 2020
@build build requested review from a team May 5, 2020 00:16
@build build added the docs label May 5, 2020
@build
Copy link
Contributor

build commented May 5, 2020

Fails
🚫 Tests have failed, see below for more information.
Messages
📖

💾 Here's the generated SDK zipfile.

📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖 ❌ 1 tests have failed There are 1 tests failing and 698 skipped out of 7309 total tests.

Tests:

ClassnameNameTimeError
ios.ipad.Titanium.UI.SearchBar.focused (13.3)3.28
Error: timeout of 2000ms exceeded
file:///Users/build/Library/Developer/CoreSimulator/Devices/A3092E1B-86D7-4525-83BF-31A47879CA0A/data/Containers/Bundle/Application/AD52A6C7-6D13-4AD6-837B-670168BF39B9/mocha.app/ti-mocha.js:6535:53732

Generated by 🚫 dangerJS against ee912e0

examples:
- title: Parsing Localized Numeric Strings
example: |
// You can convert to/from a localized numeric string like this.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use a fenced code block now:

// this is some example code

JSExportAs(parseDecimal,
-(NSNumber *)parseDecimal
: (NSString *)text withLocaleId
: (id)localeId);
Copy link
Contributor

Choose a reason for hiding this comment

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

So I'm a bit conflicted over the use of id for the second optional argument. The JSC API doesn't give us a straight-forward way to handle optional arguments. I just tested, and if you declare it as an NSString* (which it should be), but you don't provide the arg - it converts the undefined value to "undefined" as a String. Which is clearly an annoyance (and probably why I did the same above for getString's hint arg).

So I guess we have to either declare as id like this so undefined becomes nil - or not even define the optional argument and instead pull it out of [JSContext.currentArguments].

let numericString = String.formatDecimal(numericValue);
let parsedValue = Ti.Locale.parseDecimal(numericString);
should(parsedValue).be.a.Number();
should(Math.abs(parsedValue - numericValue)).be.lessThan(Number.EPSILON);
Copy link
Contributor

Choose a reason for hiding this comment

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

Neat trick - never knew about the use of EPSILON to compare floats.

May be easier to write this as:

should(parsedValue).be.approximately(numericValue, Number.EPSILON);

See https://shouldjs.github.io/#assertion-approximately


// Remove leading spaces and plus sign. Number format will fail to parse if there.
text = text.trim();
if ((text != null) && text.startsWith("+")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't need the null check since you're already trimming without checking above. I assume our bridge layer enforces non-null if the argument is not marked optional? If not, the null check needs to move to the very top.

@ssjsamir ssjsamir self-requested a review July 9, 2020 15:04
Copy link
Contributor

@ssjsamir ssjsamir left a comment

Choose a reason for hiding this comment

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

FR Passed: Tested using the following test case below.

Test Case:

var oldNumericValue = 123.456;
var stringValue = String.formatDecimal(oldNumericValue);
var newNumericValue = Ti.Locale.parseDecimal(stringValue);
if (Math.abs(newNumericValue - oldNumericValue) < Number.EPSILON) {
 	console.log('Parsed number matches original number.');
}

var number1 = Ti.Locale.parseDecimal('1,234,567.8', 'en-US');
var number2 = Ti.Locale.parseDecimal('1.234.567,8', 'de-DE');
if (Math.abs(number1 - number2) < Number.EPSILON) {
	console.log('Parsed numbers match.');
}

var result = Ti.Locale.parseDecimal('Invalid');
if (Number.isNaN(result)) {
	console.log('Invalid string returned NaN.');
}

Result

[INFO] �� � Parsed number matches original number.

[INFO] �� � Parsed numbers match.

[INFO] �� � Invalid string returned NaN.

Test Environment

MacOS Big Sur: 11.0 Beta
Xcode: 12.0 Beta 
Java Version: 1.8.0_242
Android NDK: 21.3.6528147
Node.js: 12.18.1
""NPM":"5.0.0","CLI":"8.0.0""
iphone 8 Sim (14.0 Beta)
API230 Pixel XL2 emulator

@sgtcoolguy sgtcoolguy merged commit 6253813 into tidev:master Jul 13, 2020
@ssaddique
Copy link
Contributor

Fix verified on build 9.1.0.v20200804082025.

Test environment

OS Ver:         10.15.3
Xcode Ver:      Xcode 11.6
Appc NPM:       5.0.0
Appc CLI:       8.0.0
Node Ver:       10.17.0
Java Ver:       1.8.0_162
Emulator:      Pixel 4 (API 30)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants