UnsignedInteger+Extension + tests #98

Dennis-Cherchenko · 2016-11-23T15:43:49Z

No description provided.

bobgilmore · 2016-11-23T15:59:34Z

SwiftWisdom/Core/StandardLibrary/Numbers/UnsignedInteger+Extensions.swift

-        return self.init(ip_safely: rand)
-    }
-}
-
 // TODO: evaluate whether we should move random to be a member of CountableClosedRange


Is this still TODO, or is it now resolved one way or another?

I believe @andrewdolce is addressing this in #97

bobgilmore · 2016-11-23T15:59:50Z

SwiftWisdom/Core/StandardLibrary/Numbers/UnsignedInteger+Extensions.swift

@@ -26,16 +18,46 @@ public func random(inRange range: CountableClosedRange<Int>) -> Int {

 // TODO: write tests for this extension


Is this still TODO?

bobgilmore · 2016-11-23T16:00:09Z

SwiftWisdom/Core/StandardLibrary/Numbers/UnsignedInteger+Extensions.swift

        let hexInt = ip_data.ip_hexInt ?? 0
        self.init(ip_safely: hexInt)
    }
-
-    public init(ip_data: Data) {
+


Unneeded whitespace

bobgilmore · 2016-11-23T16:02:18Z

SwiftWisdom/Core/StandardLibrary/Numbers/UnsignedInteger+Extensions.swift

-    /// Converts a bit mask into its given indexes. For example, `0b101` will return `[0,2]`
+    /// Converts a bit mask into its given indexes. For example, `0b10011` will return `[0, 1, 4]`
+
+    //Converts a bit mast into it's given indexs


Typos: "mast" -> "mask", "it's" -> "its", and "indexs" -> "indexes" (or "indices," but we use "indexes" on line 75.

bobgilmore · 2016-11-23T16:02:42Z

SwiftWisdomTests/StandardLibrary/Numbers/UnsignedInteger+ExtensionsTests.swift

+        //XCTAssertFalse()
+    }
+
+    //Finished


Unnecessary comment.

bobgilmore · 2016-11-23T16:02:48Z

SwiftWisdomTests/StandardLibrary/Numbers/UnsignedInteger+ExtensionsTests.swift

+        XCTAssertFalse(number == UInt8(0))
+    }
+
+    //Finished


Unnecessary comment.

bobgilmore · 2016-11-23T16:02:55Z

SwiftWisdomTests/StandardLibrary/Numbers/UnsignedInteger+ExtensionsTests.swift

+        XCTAssertFalse(number == UInt8(0))
+    }
+
+    //Finished


Unnecessary comment.

bobgilmore · 2016-11-23T16:03:01Z

SwiftWisdomTests/StandardLibrary/Numbers/UnsignedInteger+ExtensionsTests.swift

+        XCTAssertFalse(number == UInt(0))
+    }
+
+    //Finished


Unnecessary comment.

bobgilmore · 2016-11-23T16:03:08Z

SwiftWisdomTests/StandardLibrary/Numbers/UnsignedInteger+ExtensionsTests.swift

+        XCTAssert(UInt(UINT8_MAX).ip_containsBitMask(0))
+    }
+
+    //Finished


Unnecessary comment.

bobgilmore · 2016-11-23T16:03:13Z

SwiftWisdomTests/StandardLibrary/Numbers/UnsignedInteger+ExtensionsTests.swift

+        XCTAssertFalse(correctData == UInt8(0).ip_data)
+    }
+
+    //Finished


Unnecessary comment.

bobgilmore · 2016-11-23T16:03:17Z

SwiftWisdomTests/StandardLibrary/Numbers/UnsignedInteger+ExtensionsTests.swift

+        XCTAssertFalse(correctNSData == UInt8(0).ip_nsdata)
+    }
+
+    //Finished


Unnecessary comment.

bobgilmore · 2016-11-23T16:03:58Z

SwiftWisdomTests/StandardLibrary/Numbers/UnsignedInteger+ExtensionsTests.swift

@@ -19,4 +19,207 @@ class UnsignedIntegerExtensionTests: XCTestCase {
        let mixed = -100...100
        XCTAssert(mixed.contains(random(inRange: mixed)))
    }
+
+    func testRandom() {


Remove, or implement, or add a TODO?

bobgilmore · 2016-11-23T16:04:09Z

SwiftWisdomTests/StandardLibrary/Numbers/UnsignedInteger+ExtensionsTests.swift

+        XCTAssert(number == UInt(UINT8_MAX))
+    }
+
+    func test_ip_random() {


Remove, or implement, or add a TODO?

bobgilmore · 2016-11-23T16:04:39Z

SwiftWisdomTests/StandardLibrary/Numbers/UnsignedInteger+ExtensionsTests.swift

+        XCTAssertFalse(UInt(INT8_MAX).ip_maskedIndexes[1] == 2)
+    }
+
+    func test_ip_maxValue() {


Remove, or implement, or add a TODO for this and the next two, similar methods?

bobgilmore · 2016-11-23T16:06:51Z

SwiftWisdomTests/StandardLibrary/Numbers/UnsignedInteger+ExtensionsTests.swift

+    func test_ip_initData() {
+        var data = Data(byte: UInt8(0))
+        var number = UInt8.init(ip_data: data)
+        XCTAssert(number == UInt8(0))


This, and all similar tests throughout the PR, should be calls to XCTAssertEqual

bobgilmore · 2016-11-23T16:07:28Z

SwiftWisdomTests/StandardLibrary/Numbers/UnsignedInteger+ExtensionsTests.swift

+        var data = Data(byte: UInt8(0))
+        var number = UInt8.init(ip_data: data)
+        XCTAssert(number == UInt8(0))
+        XCTAssertFalse(number == UInt8(1))


Similarly, this and all similar tests throughout the PR, should be calls to XCTAssertNotEqual

paulrolfe · 2016-11-23T16:10:56Z

SwiftWisdom/Core/StandardLibrary/Numbers/UnsignedInteger+Extensions.swift

-
-    public init(ip_data: Data) {
+    
+    public init(ip_data: NSData) {


You just flipped where these initializers were in the file. Just pointing that out, seems like an accident?

paulrolfe · 2016-11-23T16:12:54Z

SwiftWisdom/Core/StandardLibrary/Numbers/UnsignedInteger+Extensions.swift

+        return self.init(ip_safely: rand)
+    }
+
+    public func ip_containsMask(_ mask: Self) -> Bool {


This is confusing. What's the difference between this and public func ip_containsBitMask(_ bitMask: Self)?

Oh I see you just moved it up from below, but still I have to question why we need both. I believe [client] is the only group making use of these and we prefer ip_containsBitMask

Not sure what the difference is. I received it like this and it seems like both are used in a project somewhere

I understand that both were in there. I just don't like having two functions that do literally the exact same thing, so we should pick one. I'm casting my vote for ip_containsBitMask to stay: it's used heavily in [client] and BitMask is a more descriptive term than Mask.

I agree. Should I make the change?

bobgilmore · 2016-11-23T16:27:52Z

SwiftWisdomTests/StandardLibrary/Numbers/UnsignedInteger+ExtensionsTests.swift

+    }
+
+    func test_ip_maskedIndexes() {
+        XCTAssert(UInt(0).ip_maskedIndexes == [])


The calls in this method should also get the XCTAssertEqual / XCTAssertNotEqual treatment.

bobgilmore · 2016-11-23T16:38:41Z

Not ready for merge yet, but when it is, might want to squash this down into only a few commits.

brightredchilli · 2016-11-23T18:36:50Z

SwiftWisdom/Core/StandardLibrary/Numbers/UnsignedInteger+Extensions.swift

+    // TODO: write tests for this function
+    public static func ip_random() -> Self {
+        let intMax = Int(ip_maxValue.toIntMax())
+        let rand = random(inRange: 0...intMax)


add another TODO here, which is to use @andrewdolce's new collection ip_random method when it comes out.

brightredchilli · 2016-11-23T18:37:12Z

SwiftWisdom/Core/StandardLibrary/Numbers/UnsignedInteger+Extensions.swift

    public static var ip_maxValue: Self {
        return ip_bitStackOfLength(ip_maximumNumberOfBits)
    }
-
+    


additional spaces, should clean out

brightredchilli · 2016-11-23T18:37:32Z

SwiftWisdomTests/StandardLibrary/Numbers/UnsignedInteger+ExtensionsTests.swift

@@ -19,4 +19,188 @@ class UnsignedIntegerExtensionTests: XCTestCase {
        let mixed = -100...100
        XCTAssert(mixed.contains(random(inRange: mixed)))
    }
+
+    // TODO: Write test
+    func testRandom() {


empty method should be remoed

brightredchilli · 2016-11-23T18:39:45Z

SwiftWisdomTests/StandardLibrary/Numbers/UnsignedInteger+ExtensionsTests.swift

+        var data = Data(byte: UInt8(0))
+        var number = UInt8.init(ip_data: data)
+        XCTAssertEqual(number, UInt8(0))
+        XCTAssertNotEqual(number, UInt8(1))


This not equal case is here, and in the rest of the files, are redundant. This is because XCTAssertNotEqual is simply an alias to XCTAssertEqual with the assertion flipped from true to false. IF XCTAssertEqual is true, then number can't be anything else. I'd like some other people to weigh in on this though.

Good catch @brightredchilli , the Equal check is sufficient.

brightredchilli · 2016-11-23T18:39:58Z

SwiftWisdomTests/StandardLibrary/Numbers/UnsignedInteger+ExtensionsTests.swift

+    }
+
+    func test_ip_containsMask() {
+        XCTAssert(UInt(0).ip_containsMask(0)) // 0b0, 0b0 -> true


nice tests.

brightredchilli

Really great job doing excellent testing here - some more aesthetic things I really think we need to close out before merging but this looks great.

brightredchilli · 2016-12-21T14:17:56Z

SwiftWisdom/Core/StandardLibrary/Numbers/UnsignedInteger+Extensions.swift

+    }
+
+    public init<T : UnsignedInteger>(ip_safely value: T) {
+        self = 0


this self = 0 is not necessary because if and else block seems to cover it

brightredchilli · 2016-12-21T14:23:22Z

SwiftWisdomTests/StandardLibrary/Numbers/UnsignedInteger+ExtensionsTests.swift

+
+    func test_ip_initData() {
+        var data = Data(byte: UInt8(0))
+        var number = UInt8.init(ip_data: data)


let's remove .init from this test and the rest of the tests in this diff - UInt8(...) should do.

… declaration

Fixed incorrect tests

Addressed PR comments 2 Addressed PR comments 3 Fixed incorrect function name

brightredchilli · 2017-02-17T20:12:42Z

SwiftWisdom/Core/StandardLibrary/Numbers/UnsignedInteger+Extensions.swift

 extension UnsignedInteger {

    public init(ip_data: Data) {
        let hexInt = ip_data.ip_hexInt ?? 0
        self.init(ip_safely: hexInt)
    }
-
+
+    public init<T : SignedInteger>(ip_safely value: T) {


does anyone have context as to why these two intializers would need to be introduced? do we need them?

I see that ip_safely appears now and then in files in the project, but doesn't seem to be explained anywhere. Can someone explain the convention?

They are used to avoid overflow/underflow when converting between types. I'm potentially using one as part of the ip_random() function for my other PR (in progress.)

should we not encourage this? Isn't that glossing over a potential problem?

For example, here is a test that i wrote that exercises this code:

number = UInt(ip_safely: -1) XCTAssertEqual(number, UInt(0))

this seems rather.....surprising? I feel like this should be a faillable initializer

Yes, I agree with @brightredchilli (unless that 's the point of the whole thing :-) )

andrewdolce · 2017-05-05T19:24:15Z

Closing because I let this get really out-of-date.

bobgilmore reviewed Nov 23, 2016

View reviewed changes

paulrolfe reviewed Nov 23, 2016

View reviewed changes

bobgilmore reviewed Nov 23, 2016

View reviewed changes

brightredchilli reviewed Nov 23, 2016

View reviewed changes

Dennis-Cherchenko force-pushed the evaluate-api-for-randomInRange branch 2 times, most recently from 4f64195 to 9c1b1d3 Compare November 29, 2016 14:16

brightredchilli approved these changes Dec 21, 2016

View reviewed changes

andrewdolce self-assigned this Feb 14, 2017

Dennis-Cherchenko added 3 commits February 17, 2017 14:44

Moved multiple declarations of "extension UnsignedInteger{}" into one…

46b871f

… declaration

Added thorough tests to Unsigned Integer and refactored code

d7840c3

Fixed incorrect tests

Addressed PR comments

433aa79

Addressed PR comments 2 Addressed PR comments 3 Fixed incorrect function name

brightredchilli force-pushed the evaluate-api-for-randomInRange branch from 9c1b1d3 to 433aa79 Compare February 17, 2017 19:46

removed init from tests

1a1888b

brightredchilli added 2 commits February 17, 2017 15:11

bump podfile

2c079fd

pr comments

90d4b44

brightredchilli reviewed Feb 17, 2017

View reviewed changes

andrewdolce closed this May 5, 2017

tzm41 deleted the evaluate-api-for-randomInRange branch June 20, 2017 19:34

		@@ -26,16 +18,46 @@ public func random(inRange range: CountableClosedRange<Int>) -> Int {

		// TODO: write tests for this extension

UnsignedInteger+Extension + tests #98

UnsignedInteger+Extension + tests #98

Conversation

Dennis-Cherchenko commented Nov 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulrolfe Nov 23, 2016 • edited by bobgilmore Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulrolfe Nov 23, 2016 • edited by bobgilmore Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobgilmore commented Nov 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brightredchilli Nov 23, 2016 • edited Loading

Choose a reason for hiding this comment

bobgilmore Nov 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brightredchilli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brightredchilli Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewdolce commented May 5, 2017

paulrolfe Nov 23, 2016 •

edited by bobgilmore

Loading

paulrolfe Nov 23, 2016 •

edited by bobgilmore

Loading

brightredchilli Nov 23, 2016 •

edited

Loading

bobgilmore Nov 23, 2016 •

edited

Loading

brightredchilli Feb 17, 2017 •

edited

Loading