[EnergyFormatter] Implementation and tests #791

kweinmeister · 2017-01-12T18:22:13Z

Implemented all EnergyFormatter functions and created tests. Tested on macOS and Ubuntu 16.04.

phausler · 2017-01-12T18:28:47Z

TestFoundation/TestNSEnergyFormatter.swift

+    import SwiftXCTest
+#endif
+
+class TestNSEnergyFormatter: XCTestCase {


Running these tests against the objective-c foundation give a few errors:

Test Suite 'TestNSEnergyFormatter' started at 2017-01-12 10:26:10.618 Test Case '-[TestNSEnergyFormatter test_unitStringFromJoules]' started. TestNSEnergyFormatter.swift:88: error: -[TestNSEnergyFormatter test_unitStringFromJoules] : XCTAssertEqual failed: ("kcal") is not equal to ("kCal") - TestNSEnergyFormatter.swift:91: error: -[TestNSEnergyFormatter test_unitStringFromJoules] : XCTAssertEqual failed: ("kcal") is not equal to ("kCal") - TestNSEnergyFormatter.swift:100: error: -[TestNSEnergyFormatter test_unitStringFromJoules] : XCTAssertEqual failed: ("kcal") is not equal to ("kCal") - TestNSEnergyFormatter.swift:103: error: -[TestNSEnergyFormatter test_unitStringFromJoules] : XCTAssertEqual failed: ("kcal") is not equal to ("kCal") - Test Case '-[TestNSEnergyFormatter test_unitStringFromJoules]' failed (0.051 seconds). Test Case '-[TestNSEnergyFormatter test_stringFromValue]' started. TestNSEnergyFormatter.swift:72: error: -[TestNSEnergyFormatter test_stringFromValue] : XCTAssertEqual failed: ("0kcal") is not equal to ("0kCal") - TestNSEnergyFormatter.swift:79: error: -[TestNSEnergyFormatter test_stringFromValue] : XCTAssertEqual failed: ("987,654,321 Cal") is not equal to ("987,654,321 cal") - TestNSEnergyFormatter.swift:82: error: -[TestNSEnergyFormatter test_stringFromValue] : XCTAssertEqual failed: ("5.3 kcal") is not equal to ("5.3 kCal") - Test Case '-[TestNSEnergyFormatter test_stringFromValue]' failed (0.001 seconds). Test Case '-[TestNSEnergyFormatter test_stringFromJoulesJoulesRegion]' started. TestNSEnergyFormatter.swift:42: error: -[TestNSEnergyFormatter test_stringFromJoulesJoulesRegion] : XCTAssertEqual failed: ("0 J") is not equal to ("0 J") - TestNSEnergyFormatter.swift:43: error: -[TestNSEnergyFormatter test_stringFromJoulesJoulesRegion] : XCTAssertEqual failed: ("1 J") is not equal to ("1 J") - Test Case '-[TestNSEnergyFormatter test_stringFromJoulesJoulesRegion]' failed (0.003 seconds). Test Case '-[TestNSEnergyFormatter test_stringFromJoulesCaloriesRegion]' started. TestNSEnergyFormatter.swift:49: error: -[TestNSEnergyFormatter test_stringFromJoulesCaloriesRegion] : XCTAssertEqual failed: ("-2.39 kcal") is not equal to ("-2.39 kCal") - TestNSEnergyFormatter.swift:53: error: -[TestNSEnergyFormatter test_stringFromJoulesCaloriesRegion] : XCTAssertEqual failed: ("2.39 kcal") is not equal to ("2.39 kCal") - Test Case '-[TestNSEnergyFormatter test_stringFromJoulesCaloriesRegion]' failed (0.002 seconds). Test Case '-[TestNSEnergyFormatter test_stringFromJoulesCaloriesRegionFoodEnergyUse]' started. TestNSEnergyFormatter.swift:58: error: -[TestNSEnergyFormatter test_stringFromJoulesCaloriesRegionFoodEnergyUse] : XCTAssertEqual failed: ("-0 Cal") is not equal to ("-0 cal") - TestNSEnergyFormatter.swift:62: error: -[TestNSEnergyFormatter test_stringFromJoulesCaloriesRegionFoodEnergyUse] : XCTAssertEqual failed: ("2.39 Cal") is not equal to ("2.39 cal") - Test Case '-[TestNSEnergyFormatter test_stringFromJoulesCaloriesRegionFoodEnergyUse]' failed (0.001 seconds). Test Case '-[TestNSEnergyFormatter test_unitStringFromValue]' started. TestNSEnergyFormatter.swift:134: error: -[TestNSEnergyFormatter test_unitStringFromValue] : XCTAssertEqual failed: ("Cal") is not equal to ("cal") - TestNSEnergyFormatter.swift:135: error: -[TestNSEnergyFormatter test_unitStringFromValue] : XCTAssertEqual failed: ("Cal") is not equal to ("cal") - Test Case '-[TestNSEnergyFormatter test_unitStringFromValue]' failed (0.001 seconds). Test Suite 'TestNSEnergyFormatter' failed at 2017-01-12 10:26:10.844. Executed 6 tests, with 15 failures (0 unexpected) in 0.060 (0.227) seconds Test Suite 'xctest' failed at 2017-01-12 10:26:10.915. Executed 6 tests, with 15 failures (0 unexpected) in 0.060 (0.298) seconds Test Suite 'Selected tests' failed at 2017-01-12 10:26:10.916. Executed 6 tests, with 15 failures (0 unexpected) in 0.060 (0.299) seconds

kweinmeister · 2017-01-12T18:39:28Z

Thanks @phausler! I saw some capitalization inconsistencies in the Objective-C foundation (sometimes using "cal" and in other cases "Cal" for instance), but I will go ahead and modify the Swift foundation to match exactly.

Any idea what's going on with these failed tests? Maybe I'm missing something obvious, but I can't tell what's off.

TestNSEnergyFormatter.swift:42: error: -[TestNSEnergyFormatter test_stringFromJoulesJoulesRegion] : XCTAssertEqual failed: ("0 J") is not equal to ("0 J") - 
TestNSEnergyFormatter.swift:43: error: -[TestNSEnergyFormatter test_stringFromJoulesJoulesRegion] : XCTAssertEqual failed: ("1 J") is not equal to ("1 J")

kweinmeister · 2017-01-12T19:09:41Z

EDIT: I don't think there's an issue here. For food energy use, Big C "Calories" are used, which are equivalent to kilocalories. Now I've spent a little more time in this class, I see why "calories," "Calories," and "kilocalories" are used in different contexts.

...

Just FYI, here's what one of the test functions looks like after being modified to pass with Objective-C Foundation:

    func test_stringFromJoulesCaloriesRegionFoodEnergyUse() {
        formatter.isForFoodEnergyUse = true
        XCTAssertEqual(formatter.string(fromJoules: -1), "-0 Cal")
        XCTAssertEqual(formatter.string(fromJoules: 0.001), "0 cal")
        XCTAssertEqual(formatter.string(fromJoules: 0.1), "0.024 cal")
        XCTAssertEqual(formatter.string(fromJoules: 1), "0.239 cal")
        XCTAssertEqual(formatter.string(fromJoules: 10000), "2.39 Cal")
    }

A few interesting things to note:

Negative values use "Cal" whereas some positive values use "cal"
Higher positive values that are in the kilocalories range, which presumably go through a different code path due to the isForFoodEnergyUse flag being true, also use "Cal"
UnitEnergy.Symbol (defined in Unit.swift) define the calorie symbol as static let calories = "cal"
Slightly negative numbers that are smaller than the precision cutoff (i.e. 0.000001 when maximumFractionDigits is set to 3) are being formatted in the NumberFormatter as "-0" rather than "0". This is reflected in the first test above.
When isForFoodEnergyUse is true and kicks in when there are kilocalories, only the unit changes but not the value. This is shown in the last test. 10000 joules is displayed as 2.39 calories rather than 2.39 kilocalories.

parkera · 2017-01-12T19:38:41Z

cc @lifeissweetgood

kweinmeister · 2017-01-12T21:12:35Z

I made updates to both the implementation and tests per the above feedback. I used the same tests in both my Swift Foundation workspace and in a sample iOS project and both passed 100%.

xwu · 2017-01-14T06:25:46Z

Foundation/NSEnergyFormatter.swift

+        fileprivate var singularString: String {
+            switch self {
+            case .joule:
+                return "joule"


This is missing the meat of what EnergyFormatter provides on Apple platforms: namely, formatting energy values in a locale-appropriate way. On macOS:

let formatter = EnergyFormatter() formatter.numberFormatter.locale = Locale(identifier: "de_DE") formatter.unitStyle = .long formatter.string(fromValue: 0, unit: .joule) // "0 Joule" formatter.numberFormatter.locale = Locale(identifier: "fr_FR") formatter.string(fromValue: 0, unit: .joule) // "0 joule" formatter.numberFormatter.locale = Locale(identifier: "zh_CN") formatter.string(fromValue: 0, unit: .joule) // "0焦耳"

We still need to identify a roadmap forward to handle localization bundles and the content of those for swift-corelibs-foundation. Since the environment is different (there is no "system" version of Foundation) we have to consider a methodology to store the data without relying on a common path (the bundle of the Foundation.framework). So I would say that the localization data is probably more an additional task than just the formatters. Thankfully code is mutable and we can change it after additions like this to accommodate for improvements in localizations.

Certainly. In the meantime, however, is it appropriate for a formatter that is unable to localize for some particular locales and unit styles offer known incorrect output when asked, or should the code check for supported locales and otherwise bail with NSUnimplemented()?

fallback localizations should go to en_US_POSIX so even though the current implementations may not have parity they are technically "correct" in their fallback behavior. It might be nice however to somehow have a non-failing version that would report these cases.

I see. Surely, that means that the whole thing should fall back to en_US_POSIX and not that the number should be localized and an English word appended?

In summary, the current implementation:

Formats the number based on the locale

Selects the appropriate unit based on the locale

Does not currently use a translated unit symbol

In writing this, I referenced Unit.swift, and it currently also provides English-only symbols at the moment.

Shall we proceed with this initial implementation? If not, please let me know which of the above behaviors and anything else that should be changed.

I'm ok with that, and maybe we should reflect that status in our status markdown file. Do the tests pass on macOS now?

Yes, the test pass on macOS with the 2nd commit.

That's a good idea to reflect the translation status in the markdown file. It would support transparency and put more attention on resolving this technical debt.

…bs-foundation into energyformatter

…bs-foundation into energyformatter # Conflicts: # TestFoundation/main.swift

kweinmeister · 2017-02-10T17:00:37Z

I've resolved the merge conflicts; can this be merged now?

parkera · 2017-02-10T21:08:41Z

@swift-ci please test and merge

Initial implementation of EnergyFormatter

f9c22ca

phausler reviewed Jan 12, 2017

View reviewed changes

Updates to implementation and tests to match Objective-C foundation.

43f4e92

xwu reviewed Jan 14, 2017

View reviewed changes

jessesquires mentioned this pull request Jan 15, 2017

[53] Issue #53 - January 19, 2017 SwiftWeekly/swiftweekly.github.io#165

Closed

Karl Weinmeister added 2 commits January 16, 2017 13:02

Merge branch 'master' of https://github.com/kweinmeister/swift-coreli…

e2bc6f2

…bs-foundation into energyformatter

Merge branch 'master' of https://github.com/kweinmeister/swift-coreli…

0920e69

…bs-foundation into energyformatter # Conflicts: # TestFoundation/main.swift

swift-ci merged commit 99c6656 into swiftlang:master Feb 10, 2017

jszumski mentioned this pull request Feb 16, 2017

Initial implementation of MassFormatter. #883

Merged

swift-ci mentioned this pull request Jan 12, 2017

[SR-1602] NSEnergyFormatter needs to be implemented #4149

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[EnergyFormatter] Implementation and tests #791

[EnergyFormatter] Implementation and tests #791

Uh oh!

kweinmeister commented Jan 12, 2017

Uh oh!

phausler Jan 12, 2017

Uh oh!

kweinmeister commented Jan 12, 2017 •

edited

Loading

Uh oh!

kweinmeister commented Jan 12, 2017 •

edited

Loading

Uh oh!

parkera commented Jan 12, 2017

Uh oh!

kweinmeister commented Jan 12, 2017

Uh oh!

xwu Jan 14, 2017

Uh oh!

phausler Jan 14, 2017

Uh oh!

xwu Jan 14, 2017

Uh oh!

phausler Jan 14, 2017

Uh oh!

xwu Jan 14, 2017

Uh oh!

kweinmeister Jan 24, 2017 •

edited

Loading

Uh oh!

parkera Jan 25, 2017

Uh oh!

kweinmeister Jan 25, 2017

Uh oh!

kweinmeister commented Feb 10, 2017

Uh oh!

parkera commented Feb 10, 2017

Uh oh!

Uh oh!

[EnergyFormatter] Implementation and tests #791

[EnergyFormatter] Implementation and tests #791

Uh oh!

Conversation

kweinmeister commented Jan 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kweinmeister commented Jan 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kweinmeister commented Jan 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parkera commented Jan 12, 2017

Uh oh!

kweinmeister commented Jan 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kweinmeister Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kweinmeister commented Feb 10, 2017

Uh oh!

parkera commented Feb 10, 2017

Uh oh!

Uh oh!

kweinmeister commented Jan 12, 2017 •

edited

Loading

kweinmeister commented Jan 12, 2017 •

edited

Loading

kweinmeister Jan 24, 2017 •

edited

Loading