Implement NSMeasurement #1425

Dante-Broggi · 2018-02-08T02:48:39Z

This is implementing the NSUnimplemented methods.

I am thuroughly unsure of the keys (`"NS.dblval"` and `"NS.unit"`) I used in these methods.

ianpartridge · 2018-02-08T10:21:21Z

Thanks for your contribution! Can you add some tests too?

P.S. We use 4 spaces for indentation here.

parkera · 2018-02-08T17:58:45Z

Foundation/NSMeasurement.swift

+      guard aCoder.allowsKeyedCoding else {
+            preconditionFailure("Unkeyed coding is unsupported.")
+        }
+        aCoder.encode(self.doubleValue, forKey:"NS.dblval")


Dante-Broggi · 2018-02-08T19:52:28Z

For tests, as there isn't a TestNSMeasurement.swift file, do I have to do anything special when adding it?

millenomi · 2018-02-08T20:17:10Z

Please add it to the Xcode project and the list of test source files in the build.py file when you do.

Tests don't actually test anything yet.

Dante-Broggi · 2018-02-08T21:42:06Z

The build.py includes + glob.glob('./TestFoundation/Test*.swift')) # all TestSomething.swift are considered sources to the test project in the TestFoundation directory so, actually I don't need to add TestNSMeasurement.swift, but I did add it to the Xcode project.

spevans · 2018-02-09T10:00:55Z

@swift-ci please test

spevans · 2018-02-09T11:36:23Z

Looks like there were a couple of compilation errors, see https://ci.swift.org/view/Pull%20Request/job/swift-corelibs-foundation-PR-Linux/635/consoleFull#18229161593122a513-f36a-4c87-8ed7-cbc36a1ec144

I really should not have needed CI to find these, oh well.

spevans · 2018-02-09T14:02:04Z

@swift-ci please test

spevans · 2018-02-09T14:31:07Z

@Dante-Broggi there are still a couple of compile errors in CI: https://ci.swift.org/view/Pull%20Request/job/swift-corelibs-foundation-PR-Linux/636/console

Dante-Broggi · 2018-02-11T01:58:38Z

So, the one thing I couldn't fix is canBeConverted(to:), the original implementation of which used objc-only isKind(of:) and I cannot think of how to replace that.
The nearest I can think of is (otherUnit is type(of: unit)), except is would require a dynamic metatype argument on the rhs, and it currently only accepts static metatypes.

`canBeConverted(to:)` uses `isKind(of:)` which is only available with objc.

Dante-Broggi · 2018-02-11T02:26:39Z

I thought of something that doesn't use .isKind(of:) for canBeConverted(to:)
just attempt the conversion, and return false instead of fatalError()ing.

spevans · 2018-02-11T07:37:30Z

@swift-ci please test

spevans · 2018-02-11T11:57:37Z

Foundation/NSMeasurement.swift

+            return Measurement(value: self.doubleValue + rhs.value, unit: self.unit)
+        } else {
+            guard let dimension = unit as? Dimension,
+                    let otherDimension = otherUnit as? Dimension else {


Do you mean rhs instead of otherUnit ? (same on line 82)

Yes, indeed.

spevans · 2018-02-11T11:59:02Z

Foundation/NSMeasurement.swift

+            } else {
+                return unit.isEqual(otherUnit)
+            }
+            re


Extra typo here

spevans · 2018-02-11T12:59:21Z

@swift-ci please test

spevans · 2018-02-11T14:19:19Z

@swift-ci please test

spevans · 2018-02-11T14:50:14Z

Foundation/NSMeasurement.swift

+            guard let dimension = unit as? Dimension,
+                    let otherDimension = rhs.unit as? Dimension else {
+                        fatalError("Cannot convert differing units that are non-dimensional! lhs: \(type(of: dimension)) rhs: \(type(of: otherDimension))")
+                }


This needs to be (type(of: unit)) and (type(of: rhs.unit)) as dimension and otherDimension don't exist in th guard block if the as? fails.

btw, are you testing on macOS or Linux?

I am on a mac, but I really have no idea how to actually even build any of the swift projects.
So, realistically, nowhere. I am sorry, but I am just an enthusiast with no idea how to use the infrastructure. 🙁

spevans · 2018-02-11T14:52:03Z

Foundation/NSMeasurement.swift

+            // just check conversion
+            if let dimension = unit as? Dimension,
+               let otherDimension = otherUnit as? Dimension {
+                return true


dimension and otherDimension are not used so could be replaced with _ or perhaps rewrite as unit is Dimension && otherUnit is Dimension.

I think I thought of that, then forgot to do it.

spevans · 2018-02-11T16:14:17Z

@Dante-Broggi Ok here's what to do assuming you are on either Sierra (10.12) or High Sierra (10.13) and have Xcode already installed:

Get the latest snapshot from https://swift.org/download/#snapshots Trunk development (master) 'Xcode' . This is a package you install on your mac.

Next, in Xcode, goto Xcode > Toolchains and you should see the snapshot you just downloaded in the menu, select it as your default tool chain. It will be called something like 'Swift Development Snapshot 2018-02-08 (a)' or something like that.

Now in the same directory that you checked out the swift-corelibs-foundation repo you also need to checkout the swift-corelibs-xctest repo.

Now in the swift-corelibs-foundation directory, open the Foundation.xcworkspace workspace (NOT the xcodeproj). The workspace includes the Foundation Xcode project and the XCTest Xcode project.

In the menu at the top the Xcode window, select 'TestFoundation > My Mac' then do 'Product > Run' and that will compile up CodeFoundation, Swift Foundation, XCtest and TestFoundation and then it will try and run the tests. Any compile errors should show up here. Don't worry about all of the warnings about deprecation in code that you haven't changed, there is usually because of changes in the main swift core in the snapshot you downloaded.

That should be enough to get you started, let me know if you have any questions about the above.

spevans · 2018-02-11T20:08:17Z

@swift-ci please test

xwu · 2018-02-11T22:08:46Z

Foundation/NSMeasurement.swift

@@ -23,21 +23,93 @@ open class NSMeasurement : NSObject, NSCopying, NSSecureCoding {
        self.unit = unit
    }

-    open func canBeConverted(to unit: Unit) -> Bool { NSUnimplemented() }
+    open func canBeConverted(to otherUnit: Unit) -> Bool {
+        #if DEPLOYMENT_RUNTIME_OBJC //|| os(Linux)


Nit: the stuff inside a compilation conditional should be indented as though there weren't a conditional surrounding it; in most files, the conditional statement itself should be outdented to the left margin, but the specific style can vary depending on the file.

xwu · 2018-02-11T22:09:08Z

Foundation/NSMeasurement.swift

+            } else {
+                return unit.isEqual(otherUnit)
+            }
+        #endif


Does this implementation match Darwin Foundation? It seems to be very limited.

xwu · 2018-02-11T22:09:45Z

Foundation/NSMeasurement.swift

+            } else {
+                guard let dimension = unit as? Dimension,
+                    let otherDimension = otherUnit as? Dimension else {
+                        fatalError("Cannot convert differing units that are non-dimensional! lhs: \(type(of: unit)) rhs: \(type(of: otherUnit))")


Try to align these error messages as closely as possible with Darwin Foundation.

xwu · 2018-02-11T22:10:40Z

Foundation/NSMeasurement.swift

+                return Measurement(value: doubleValue, unit: otherUnit)
+            } else {
+                guard let dimension = unit as? Dimension,
+                    let otherDimension = otherUnit as? Dimension else {


Nit: the stuff inside the braces should be indented only four spaces as compared to guard; it's fine to indent the second line of the condition but it doesn't affect how the inside stuff is indented (the same applies elsewhere below).

xwu · 2018-02-11T22:11:07Z

Foundation/NSMeasurement.swift

+        guard aCoder.allowsKeyedCoding else {
+            preconditionFailure("Unkeyed coding is unsupported.")
+        }
+        aCoder.encode(self.doubleValue, forKey:"NS.value")


Nit: space after forKey:; likewise one line below.

xwu · 2018-02-11T22:11:27Z

Foundation/NSMeasurement.swift

+        let doubleValue = aDecoder.decodeDouble(forKey: "NS.value")
+        let possibleUnit = aDecoder.decodeObject(forKey: "NS.unit")
+        guard let unit = possibleUnit as? Unit else {
+            return nil // or should we `fatalError()`?


What does Darwin Foundation do?

One thing it does is validation, e.g. @"Cannot add measurements of differing unit types!"

alblue · 2018-08-07T12:08:02Z

@swift-ci please test

millenomi · 2018-10-10T19:16:31Z

@Dante-Broggi are you still interested in chaperoning this patch?

ikesyo · 2019-05-25T01:29:43Z

Looks like this is superseded by #2095.

Dante-Broggi added 6 commits February 7, 2018 18:55

Implement adding(_:) and subtracting(_:)

3a135ea

Implement encode(_:) and init(coder:)

5346f25

I am thuroughly unsure of the keys (`"NS.dblval"` and `"NS.unit"`) I used in these methods.

Implement canBeConverted(to:) and converting(to:)

da6abf6

These don't return NSMeasurement

04de52e

I think NSMeasurement is immutable

902e1a9

Update Status.md

ccfeece

parkera reviewed Feb 8, 2018

View reviewed changes

Review - "NS.dblval" to "NS.value" and indentation

fa8fe19

Dante-Broggi added 2 commits February 8, 2018 16:28

NSMeasurement test case stub

2f4e5a1

Tests don't actually test anything yet.

add TestNSMeasurement.swift to XCode

fff371d

Dante-Broggi added 2 commits February 8, 2018 16:47

add TestNSMeasurement to main.swift test case list

8deb185

Add Tests, and fix other "NS.dblval"

f34df89

Fix some dumb mistakes and update Status.md for tests

0b2f8d5

I really should not have needed CI to find these, oh well.

Dante-Broggi added 2 commits February 10, 2018 21:14

Fix everything but canBeConverted(to:) and #if that out

a6a7651

`canBeConverted(to:)` uses `isKind(of:)` which is only available with objc.

I suppose this should actually work.

1e50a23

spevans reviewed Feb 11, 2018

View reviewed changes

Foundation/NSMeasurement.swift Outdated

} else {

return unit.isEqual(otherUnit)

}

re

Copy link

Contributor

spevans Feb 11, 2018

Choose a reason for hiding this comment

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

Extra typo here

More minor reviewed typos

2f195f0

Should have been more through

6161d7b

spevans reviewed Feb 11, 2018

View reviewed changes

Dante-Broggi added 2 commits February 11, 2018 09:59

these particular values are unnessasary

e9986ee

guard else errors need to use the origional errors

0f084a8

xwu reviewed Feb 11, 2018

View reviewed changes

Clean up the dumb (spelling) mistakes

b4bc5a5

ikesyo closed this May 25, 2019

Implement NSMeasurement #1425

Implement NSMeasurement #1425

Uh oh!

Conversation

Dante-Broggi commented Feb 8, 2018

Uh oh!

ianpartridge commented Feb 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dante-Broggi commented Feb 8, 2018

Uh oh!

millenomi commented Feb 8, 2018

Uh oh!

Dante-Broggi commented Feb 8, 2018

Uh oh!

spevans commented Feb 9, 2018

Uh oh!

spevans commented Feb 9, 2018

Uh oh!

spevans commented Feb 9, 2018

Uh oh!

spevans commented Feb 9, 2018

Uh oh!

Dante-Broggi commented Feb 11, 2018

Uh oh!

Dante-Broggi commented Feb 11, 2018

Uh oh!

spevans commented Feb 11, 2018

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!

spevans commented Feb 11, 2018

Uh oh!

spevans commented Feb 11, 2018

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!

spevans commented Feb 11, 2018

Uh oh!

spevans commented Feb 11, 2018

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alblue commented Aug 7, 2018

Uh oh!

millenomi commented Oct 10, 2018

Uh oh!

ikesyo commented May 25, 2019

Uh oh!

Uh oh!