Initial implementation of MassFormatter. #883
Conversation
|
Localization was not even attempted following the discussion from #791 I also didn't use |
|
|
||
|
|
||
| /// The number of pounds in 1 stone | ||
| private static let poundsPerStone = 14.0 |
There was a problem hiding this comment.
My goal was to use Measurement<UnitMass>(value:1, unit:.stones).converted(to:.pounds) here, but that ends up being 0.3471688213196 lb instead of the expected 14 lb.
There was a problem hiding this comment.
Not that I use stones regularly, but this seems like an issue :-)
Coefficient should be 6.35029? https://github.com/apple/swift-corelibs-foundation/blob/master/Foundation/Unit.swift#L1330
There was a problem hiding this comment.
@e78l Those are kilograms not pounds.
There was a problem hiding this comment.
Was referring to the 1 stone=0.347 lb conversion problem by 'this' - changing 0.157473 on L1330 of Unit.swift to 6.35029 should solve that problem.
Appears macOS 10.12.3's Foundation has the stone conversion issue too.
Hard-coding poundsPerStone = 14.0 looks great (and perhaps is the better way)!
jszumski
force-pushed
the
sr-1600-mass-formatter
branch
from
879c427 to
a72f61a
Compare
|
@lifeissweetgood Conflicts are resolved, have you had a chance to review? If you have any pointers on the best way to start the localization work I can get started on that in another branch. |
Foundation/NSMassFormatter.swift
Outdated
| /// | ||
| /// - Parameter numberInKilograms: the magnitude in terms of kilograms | ||
| /// - Returns: Returns the appropriate unit | ||
| private func unit(fromKilograms numberInKilograms: Double) -> Unit { |
There was a problem hiding this comment.
Could we change the name of this method? It's a little hard to immediately get what it's doing from the signature. Maybe something like convertedUnit(fromKilograms numberInKilograms:)?
|
In regards to pointers for localization, we're still working on figuring out the best approach for it that would work on all platforms. |
|
As an initial implementation, I think this is a pretty solid start. It leaves room for us to make improvements once we have a better story for supporting localization backed with locale data. |
jszumski
force-pushed
the
sr-1600-mass-formatter
branch
from
f46cd8e to
f306ee0
Compare
|
Updated. Thank you for the feedback! |
|
@swift-ci please test |
jszumski
force-pushed
the
sr-1600-mass-formatter
branch
from
dae0507 to
2cf8700
Compare
|
Conflicts are resolved, I think this is ready to merge if there isn't any more feedback. |
|
@swift-ci please test |
|
Looks good to me - @parkera any objections with merging in as is? |
|
Thanks everyone, let's merge it. |
| internal extension Locale { | ||
| /// TODO: Replace calls to the below function to use Locale.usesMetricSystem | ||
| /// Temporary workaround due to unpopulated Locale attributes | ||
| /// See https://bugs.swift.org/browse/SR-3202 |
Implemented MassFormatter and created tests.
Tests pass 100% on macOS and in a sample iOS project against Objective-C Foundation.